[Rt-commit] rt branch, 4.0-trunk, updated. rt-4.0.5-260-g153a17f
Alex Vandiver
alexmv at bestpractical.com
Tue May 22 12:15:52 EDT 2012
The branch, 4.0-trunk has been updated
via 153a17f9e898a744eeec45c983cdccf0055b22ea (commit)
via 650e03250271a39121eba428a41b1592d8342a79 (commit)
via ddb3ab99a6eb359394a6d9c9b5ec4d471c061601 (commit)
via 299b6604bd36c99bbffa0710a239f3ec4f60e03a (commit)
via 096e31e8f7cffcaea01b3aed91355181fee8b0bd (commit)
via b770e5f8abc6418ca8cb8e592287af535bd72249 (commit)
via 730eea81fec59cffbc3b9631b2b225c99a6cb704 (commit)
via 58c006e6719a4a93e5422074dafdbd90f0bb2a48 (commit)
via 843659b101f4aecc0fcae17dac7dd2206356ec73 (commit)
via 48ff24953c5af2efd77fe0f80490cd98aa31eb0f (commit)
via 7eaa035980cabbcb21fdbc92d9b8b4691cd735a8 (commit)
via 33840d670d3c801863a86446d1291880708a74ea (commit)
via 17bc0c17ab2523c7c73284ac8806954ad9ee573f (commit)
via 906e9a34c52d890e5d69533d9fd35c9547f0cb43 (commit)
via f9390c4962591a77046c13070019aa960e071c6d (commit)
via 7bc96758e8c01b067de13aa5d3a06509ebcab802 (commit)
via 8680717f01a26e656b74fd9ca1c9bfd1720e5519 (commit)
via d9ab0d48e09a24ecad965e021eab31366ae6b860 (commit)
via 6d390c32367a820a413defce2677cf8a3b3a1ad1 (commit)
via 6dc6a0bb9f043b5698349a0d5c946fe58029a36c (commit)
via 5c96ad518540645f3daa76111ebb07109c75c0ab (commit)
via fa9c4b4b218ea231c048312a3ca0be76b3231a1e (commit)
via 21da57aba3248b21240954274bcf5d9a47c92b49 (commit)
via 4786981af61f69a5734f7ac38b394aaab82771a4 (commit)
via e681fa720ebe8a6d6949dae6f72ccce1f06f9397 (commit)
via 4c486f95227079fcb367f1a3882feeae33edf7a1 (commit)
via 078257dc4b9da5f5575c257fd1a5f0cee044a200 (commit)
via 85142adb3b62e4d90454d28933b04ebade7b206f (commit)
via 10a3bb4c825247aeb1ffab10bb1bb0f4e40ead6c (commit)
via 4ff6192e94b193def986b970f7c219b80cd8aa9b (commit)
via dbb8542375f98daa79cf12151589d6ad0158fddf (commit)
via 09ec4163b57c60cdb42c610a77ce431fab7d787f (commit)
via b88578beb8179583acb6ee310ba0e757bef44614 (commit)
via 22bbf1944adcef38a497236ac5d691280d2b91cd (commit)
via a9bd59f450af6a3540d114f4fc9c9b148e9d5548 (commit)
via 06ea1ab348159999e5563ac72a4deecc4e203c37 (commit)
via bac33a25630ef70be3efe3635789b08f48228093 (commit)
via eb74e9568157a1027aa9c4d71fb9b38a4e3323e8 (commit)
via 0ca6a53efd94155c1fb7a2b09859156f7a05edf3 (commit)
via 855906aa2850c6277688536d7df532e25529efe6 (commit)
via c44e395e91292392fbd8d36821220b6f71b40474 (commit)
via 5242c76b43961555de802c7de26a605df34c02d0 (commit)
via 22fa8d088839c8d66c7d6311e4031aa62d7008f0 (commit)
via c68172b9a7b8e045215e70f1490145164cd00ab6 (commit)
via 1d7cc2220480f5d7e9f37994f01c1958aac960fb (commit)
via a23c3260aea61415135b35eb9efba3b52ee7187a (commit)
via 9aa0957f42d354df6d1848c7736647ef88c9e29e (commit)
via feec1c6e775de48a0c95c359ea8cc70bbf1d5538 (commit)
via 41a266405b9809d1e9dc0fc5335cf7683460b813 (commit)
via 93cb7cb1d09352627a7060e50821aca1ea5924aa (commit)
via 2b5e6c9ff6cb1aeef306d3f83887099a8036ac37 (commit)
via 475780b6817d5a1c3de54bd524e3fc7426077460 (commit)
via 8fdfef724fa75dda553679a6a30fe7d7cc60bd8b (commit)
via e47c6fbbb19790089134dee5af9c1e89bf88809b (commit)
via e8c2f511c6fcc49f1e405e054cb9cedac027fe17 (commit)
via e0ac46a7ef1cb3c61fa015ce3f2f8bcb870798b3 (commit)
via 29d4827b4b5f0060bc2e76f564a9a26d8523e226 (commit)
via b9a5e5f9b8c14ea97286484d02827bfd89169042 (commit)
via 69178f9fc6ce3aecfc827987d81ba6fc92a5e96e (commit)
via 08754c08ae211c24cdba5b8390883f65578efc95 (commit)
via 3929c48b545f5d0245a31b7c61ee90bed45549be (commit)
via bb24a9f477d792ed77ecb8bf1bc29ae958734297 (commit)
via 86dc0486708f5b778b20c3a30c138beb0cf5e489 (commit)
via 312199f66c840c444c6414815dcc186c6653278e (commit)
via fbef48d9f2271c87391c459477da1cb77d8a15b2 (commit)
via 04a9551f9a6a4a8042dc30911133ad652a79c69b (commit)
via 65ff771972e8973145fc4132dc459a0a3b53ad69 (commit)
via f258e65879c8c254c907d7d68c706d5fcea17486 (commit)
via 3ee90284f10067c9d1a29b7a1d09338e308a76be (commit)
via 87aa1d4fd8f07aaeb54cb54f23f40c935e23e897 (commit)
via 74ab1eaab2ca78c7d8b3a451167c88bcb4ec1335 (commit)
via 4881ae828fa604dc2b7df6531c93654b104f8909 (commit)
via 4209699a3f6301c3e95e70216cb80c848f8133e0 (commit)
via 3718c5ea1b1e988980a03a8bbdf93a214add5152 (commit)
via 6221350f2ca27615ed5ef6b87b1d3ff76f16463f (commit)
via 08b7989feee46bbd95d253714fb90e112d37aa3a (commit)
via 58ac3d2ebe46394d10ebdf413f287aea73f2a646 (commit)
via 59d2fb3ad38acf6614515aef0e7e2e5ba7c5634d (commit)
via f8eafa6e6bf951ffade5abf62682204b7acd2e77 (commit)
via 619d19d8f5ff9200220742db5d0352b77c9755ea (commit)
via 5a927993be1a33d1837bc7ab21836fb29206d278 (commit)
via 057463bc9914d8d6472a2a08009caefb2f8cdc53 (commit)
via 1d838609a9dfa35dc9e05b088a79cf7a5f8e8a3d (commit)
via 2ad1bcc658c38c7be44de7aff54b7199975ab5b6 (commit)
via 6ef92feaade1d8009bea08f0cb9f1ce8134714e5 (commit)
via 7d661a575463722a4d8ec7972c504b1f1829bb68 (commit)
via cb662a572320ea7df39adb87c8e6e4243bdfa95c (commit)
via c6669b25b173bcff6205f01231a9110e29b2179f (commit)
via 3a7a6d9818aa0c5cee0f0718c45d9bdbb9ff729c (commit)
via 7b181889291137eeb74fa8e140bf1db895f820be (commit)
via c0b8291e9b9f6581dc57bb55c19938d61ec77bec (commit)
via 162cd0600533c6ebfd7cfe84c36f74ece6016f47 (commit)
via 1792a7e43a5f01485f6e7ac337b1f425f50025f7 (commit)
via a2a50999aa214fa01bb824d2b6fcec197ec2a8e9 (commit)
via 928e123047291ffdad341cf4ea680e4f1ee32793 (commit)
via bb35edd1aaa63499ff5e03f2b1747c9daa334f9f (commit)
via 4faebb190e299f7b6698cf5a16fffc49d3c8ea8a (commit)
via b7eb9cbcc34931857fc2403eeab30d0663a17e72 (commit)
via d1655ade198840f1cd33690ecf1ff2172181afd0 (commit)
via b7393fb869e3ee843389e932e07a59266c4ce2a6 (commit)
via 5506d7cd5646ef95bb94ce9a1585aa69e14539e1 (commit)
via 29f7442f16352369779a43ad39a02149470032cd (commit)
via de58d4d2cf5e8742cd8ee3784f50923a19b338ae (commit)
via 8ebe790ea3271c7fedbc9fb6357aaa1f80b169ef (commit)
via 0d10462c93c0369a7c973f83b82893ec2b78af30 (commit)
via 55cb6f4032cd9a98ee650ab88515b2b8c5b09634 (commit)
via fa17a99b427c1c0a627bd144d692633b078bb1cb (commit)
via a3c69912e79951f1ef1b2df527f86d0f7ee4ca8b (commit)
via 0aaecd1166d8ed3aef066fa833eaa974190bec42 (commit)
via 5f265b6e7a59e60c6317985b9aacafc0bbd54f66 (commit)
via 303affec335d0e44bcb374ebd5cb6af862d013f7 (commit)
via c86408bba0f166786e0c48bb5e7be5126cf1039a (commit)
via dbd78716780b22733e92e8048691e560a31b8494 (commit)
via 4f0c2bd4c1a75d166937984dc2fa42bebdcf46aa (commit)
via c8466ec04ffaad0658f65cb104cde2c2a11bb499 (commit)
via c29107c60454340eaac64f00acacce8b76bc1970 (commit)
via c36e510f788d72245d0464026fe22b1489b5c1f4 (commit)
via 3141f16f93e48a0f939319d8eaf8c1411562960a (commit)
via 006cfcd255cf190a9fd71a9a9a959fe7ae50881c (commit)
via c5f4ee6a1a64209629749602b54dfd2b6588d53e (commit)
via 61dcf35da28b1be98fd8329d570af5c8308d80c0 (commit)
via e27e9174b64099c40f9546a85de51c6e9de18bcb (commit)
via 52b40c3287f54e1201c25276db74594928b4cacc (commit)
via 6c14b6bcf11a62d55db79653d2bba1d4cb47fbad (commit)
via f4badd92f323f42a58d87ccb50b93d6d9c283a37 (commit)
from 4389336ffd09f7cc94525567fc0400ad3ac2d570 (commit)
Summary of changes:
.gitignore | 1 +
bin/rt-mailgate.in | 1 -
docs/security.pod | 15 +
etc/RT_Config.pm.in | 42 +++
etc/upgrade/4.0.6/content | 17 +
etc/upgrade/vulnerable-passwords.in | 3 +
lib/RT.pm | 20 +-
lib/RT/ACL.pm | 3 +
lib/RT/Action/CreateTickets.pm | 13 +-
lib/RT/Action/SendEmail.pm | 9 +-
lib/RT/Article.pm | 11 +
lib/RT/Attachments.pm | 11 +-
lib/RT/Class.pm | 1 +
lib/RT/Config.pm | 1 +
lib/RT/CustomField.pm | 80 ++++-
lib/RT/Dashboard/Mailer.pm | 3 +
lib/RT/Date.pm | 30 +-
lib/RT/Graph/Tickets.pm | 10 +-
lib/RT/Group.pm | 10 +
lib/RT/Groups.pm | 8 +
lib/RT/Handle.pm | 6 +-
lib/RT/Interface/Email.pm | 27 +-
lib/RT/Interface/Web.pm | 361 +++++++++++++++++---
lib/RT/Interface/Web/Handler.pm | 12 +-
lib/RT/ObjectCustomField.pm | 12 +
lib/RT/ObjectCustomFieldValue.pm | 8 +-
lib/RT/Queue.pm | 12 +
lib/RT/Scrip.pm | 24 +-
lib/RT/SearchBuilder.pm | 13 +
lib/RT/Shredder.pm | 2 +
lib/RT/Shredder/Plugin.pm | 1 +
lib/RT/Shredder/Queue.pm | 1 +
lib/RT/Template.pm | 24 ++
lib/RT/Ticket.pm | 16 +-
lib/RT/Tickets.pm | 24 +-
lib/RT/Transaction.pm | 18 +-
lib/RT/URI.pm | 2 +-
lib/RT/User.pm | 74 ++--
lib/RT/Users.pm | 8 +
sbin/rt-server.in | 1 +
share/html/Admin/Articles/Elements/Topics | 2 +-
share/html/Admin/CustomFields/Modify.html | 4 +-
share/html/Admin/Elements/EditCustomFields | 3 +
share/html/Admin/Elements/EditRights | 6 +-
share/html/Admin/Elements/Portal | 2 +-
share/html/Admin/Elements/SelectNewGroupMembers | 8 +-
share/html/Admin/Groups/index.html | 2 +-
share/html/Admin/Tools/Queries.html | 4 +-
share/html/Admin/Tools/Shredder/Dumps/dhandler | 5 +-
.../Admin/Tools/Shredder/Elements/Error/NoStorage | 2 +-
share/html/Admin/Users/index.html | 2 +-
share/html/Approvals/Elements/PendingMyApproval | 4 +-
share/html/Articles/Article/Edit.html | 1 +
share/html/Articles/Article/Elements/EditTopics | 55 ++-
share/html/Articles/Article/ExtractIntoClass.html | 2 +-
share/html/Articles/Elements/ShowTopicLink | 27 ++
share/html/Articles/Topics.html | 249 +++++---------
.../Classes/GroupRights.html => Elements/CSRF} | 39 +--
share/html/Elements/CollectionAsTable/Header | 4 +-
share/html/Elements/CollectionListPaging | 12 +-
share/html/Elements/ColumnMap | 10 +-
share/html/Elements/CreateTicket | 2 +-
share/html/Elements/EditCustomField | 2 +-
share/html/Elements/EditCustomFieldAutocomplete | 13 +-
share/html/Elements/EditCustomFieldSelect | 6 +-
share/html/Elements/Error | 2 +-
share/html/Elements/Footer | 4 +-
share/html/Elements/Header | 2 +-
share/html/Elements/HeaderJavascript | 4 +-
share/html/Elements/MessageBox | 15 +-
share/html/Elements/RT__CustomField/ColumnMap | 8 +-
share/html/Elements/RT__Dashboard/ColumnMap | 2 +-
share/html/Elements/SelectOwnerAutocomplete | 4 +-
share/html/Elements/ShowCustomFields | 10 +-
share/html/Elements/ShowSearch | 6 +-
share/html/Elements/ShowUser | 2 +-
share/html/Elements/Submit | 14 +-
share/html/Elements/Tabs | 46 +--
share/html/Helpers/Autocomplete/CustomFieldValues | 44 ++-
share/html/Helpers/Toggle/ShowRequestor | 4 +-
share/html/Install/DatabaseType.html | 2 +-
share/html/Install/Finish.html | 2 +-
share/html/NoAuth/Logout.html | 2 +-
share/html/NoAuth/js/titlebox-state.js | 2 +-
share/html/NoAuth/js/userautocomplete.js | 2 +-
share/html/NoAuth/js/util.js | 4 +-
share/html/REST/1.0/Forms/transaction/default | 3 -
share/html/Search/Chart.html | 2 +-
share/html/Search/Results.html | 12 +-
share/html/Search/Simple.html | 10 +-
share/html/SelfService/Elements/MyRequests | 22 +-
share/html/SelfService/index.html | 2 +
share/html/Ticket/Elements/Bookmark | 2 +-
share/html/Ticket/Elements/ClickToShowHistory | 2 +-
share/html/Ticket/Elements/FoldStanzaJS | 2 +-
share/html/Ticket/Elements/ShowHistory | 9 +-
share/html/Ticket/Elements/ShowRequestor | 4 +-
share/html/Ticket/Elements/UpdateCc | 6 +-
.../Ticket/Graphs/Elements/EditGraphProperties | 2 +-
share/html/Ticket/Graphs/Elements/ShowGraph | 1 +
share/html/Ticket/Graphs/dhandler | 1 +
share/html/Widgets/ComboBox | 4 +-
share/html/Widgets/TitleBoxStart | 2 +-
share/html/index.html | 2 +-
share/html/l | 2 +-
share/html/{l => l_unsafe} | 0
share/html/m/_elements/footer | 2 +-
share/html/m/ticket/create | 15 +-
share/html/m/ticket/show | 12 +-
share/html/m/tickets/search | 2 +-
t/api/date.t | 10 +-
t/web/case-sensitivity.t | 2 +-
t/web/csrf-rest.t | 77 +++++
t/web/csrf.t | 181 ++++++++++
t/web/owner_disabled_group_19221.t | 190 +++++++++++
t/web/redirect-after-login.t | 6 +-
116 files changed, 1658 insertions(+), 511 deletions(-)
create mode 100644 etc/upgrade/4.0.6/content
create mode 100644 share/html/Articles/Elements/ShowTopicLink
copy share/html/{Admin/Articles/Classes/GroupRights.html => Elements/CSRF} (66%)
copy share/html/{l => l_unsafe} (100%)
create mode 100644 t/web/csrf-rest.t
create mode 100644 t/web/csrf.t
create mode 100644 t/web/owner_disabled_group_19221.t
- Log -----------------------------------------------------------------
commit f4badd92f323f42a58d87ccb50b93d6d9c283a37
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Mon Apr 4 16:05:08 2011 -0400
Remove unused GenericQueryArgs parameter
diff --git a/share/html/SelfService/Elements/MyRequests b/share/html/SelfService/Elements/MyRequests
index 8bca076..9325fdf 100755
--- a/share/html/SelfService/Elements/MyRequests
+++ b/share/html/SelfService/Elements/MyRequests
@@ -52,7 +52,6 @@
Order => @Order,
OrderBy => @OrderBy,
BaseURL => $BaseURL,
- GenericQueryArgs => $GenericQueryArgs,
AllowSorting => $AllowSorting,
Class => 'RT::Tickets',
Rows => $Rows,
@@ -79,7 +78,6 @@ $title => loc("My [_1] tickets", $friendly_status)
@status => RT::Queue->ActiveStatusArray()
$BaseURL => undef
$Page => 1
-$GenericQueryArgs => undef
$AllowSorting => 1
@Order => ('ASC')
@OrderBy => ('Created')
commit 6c14b6bcf11a62d55db79653d2bba1d4cb47fbad
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Mon Apr 4 16:05:47 2011 -0400
Similarly, there is no reason to configure AllowSorting
diff --git a/share/html/SelfService/Elements/MyRequests b/share/html/SelfService/Elements/MyRequests
index 9325fdf..2a5af8a 100755
--- a/share/html/SelfService/Elements/MyRequests
+++ b/share/html/SelfService/Elements/MyRequests
@@ -52,7 +52,7 @@
Order => @Order,
OrderBy => @OrderBy,
BaseURL => $BaseURL,
- AllowSorting => $AllowSorting,
+ AllowSorting => 1,
Class => 'RT::Tickets',
Rows => $Rows,
Page => $Page &>
@@ -78,7 +78,6 @@ $title => loc("My [_1] tickets", $friendly_status)
@status => RT::Queue->ActiveStatusArray()
$BaseURL => undef
$Page => 1
-$AllowSorting => 1
@Order => ('ASC')
@OrderBy => ('Created')
$Rows => 50
commit 52b40c3287f54e1201c25276db74594928b4cacc
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Mon Apr 4 16:06:50 2011 -0400
Disallow setting arbitrary titles
diff --git a/share/html/SelfService/Elements/MyRequests b/share/html/SelfService/Elements/MyRequests
index 2a5af8a..6e29e88 100755
--- a/share/html/SelfService/Elements/MyRequests
+++ b/share/html/SelfService/Elements/MyRequests
@@ -45,7 +45,7 @@
%# those contributions and any derivatives thereof.
%#
%# END BPS TAGGED BLOCK }}}
-<&| /Widgets/TitleBox, title => $title &>
+<&| /Widgets/TitleBox, title => $title &>
<& /Elements/CollectionList, Title => $title,
Format => $Format,
Query => $Query,
@@ -59,6 +59,7 @@
</&>
<%INIT>
+my $title = loc("My [_1] tickets", $friendly_status);
my $id = $session{'CurrentUser'}->id;
my $Query = "( "
. join( ' OR ', map "$_.id = $id", @roles )
@@ -69,11 +70,9 @@ if ( @status ) {
. " )";
}
my $Format = RT->Config->Get('DefaultSelfServiceSearchResultFormat');
-
</%INIT>
<%ARGS>
$friendly_status => loc('open')
-$title => loc("My [_1] tickets", $friendly_status)
@roles => ('Watcher')
@status => RT::Queue->ActiveStatusArray()
$BaseURL => undef
commit e27e9174b64099c40f9546a85de51c6e9de18bcb
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Mon Apr 4 16:08:14 2011 -0400
Disallow setting of roles via query params
This closes a hole wherein unprivileged users could inject TicketSQL.
While this is not in itself dangerous, it might allow unprivileged users
to see unexpected tickets if rights have been mis-applied by the
administrator (such as giving ShowTicket to Everyone).
diff --git a/share/html/SelfService/Elements/MyRequests b/share/html/SelfService/Elements/MyRequests
index 6e29e88..a7b453d 100755
--- a/share/html/SelfService/Elements/MyRequests
+++ b/share/html/SelfService/Elements/MyRequests
@@ -61,9 +61,7 @@
<%INIT>
my $title = loc("My [_1] tickets", $friendly_status);
my $id = $session{'CurrentUser'}->id;
-my $Query = "( "
- . join( ' OR ', map "$_.id = $id", @roles )
- . ")";
+my $Query = "( Watcher.id = $id )";
if ( @status ) {
$Query .= " AND ( "
. join( ' OR ', map "Status = '$_'", @status )
@@ -73,7 +71,6 @@ my $Format = RT->Config->Get('DefaultSelfServiceSearchResultFormat');
</%INIT>
<%ARGS>
$friendly_status => loc('open')
- at roles => ('Watcher')
@status => RT::Queue->ActiveStatusArray()
$BaseURL => undef
$Page => 1
commit 61dcf35da28b1be98fd8329d570af5c8308d80c0
Author: Shawn M Moore <sartak at bestpractical.com>
Date: Thu May 5 13:29:14 2011 -0400
Escape subject and links in /m/ticket/create
diff --git a/share/html/m/ticket/create b/share/html/m/ticket/create
index 5ddb6b8..a19e68e 100644
--- a/share/html/m/ticket/create
+++ b/share/html/m/ticket/create
@@ -53,6 +53,7 @@ $CloneTicket => undef
$m->callback( CallbackName => "Init", ARGSRef => \%ARGS );
my $Queue = $ARGS{Queue};
+my $escape = sub { $m->interp->apply_escapes(shift, 'h') };
my $showrows = sub {
my @pairs = @_;
@@ -263,7 +264,7 @@ if ((!exists $ARGS{'AddMoreAttach'}) and (defined($ARGS{'id'}) and $ARGS{'id'} e
<%perl>
$showrows->(
- loc("Subject") => '<input name="Subject" size="30" maxsize="200" value="'.($ARGS{Subject} || '').'" />');
+ loc("Subject") => '<input name="Subject" size="30" maxsize="200" value="'.$escape->($ARGS{Subject} || '').'" />');
</%perl>
<span class="content-label label"><%loc("Describe the issue below")%></span>
<& /Elements/MessageBox, exists $ARGS{Content} ? (Default => $ARGS{Content}, IncludeSignature => 0 ) : ( QuoteTransaction => $QuoteTransaction ), Height => 5 &>
@@ -427,12 +428,12 @@ $showrows->(
<%perl>
$showrows->(
- loc("Depends on") => '<input size="10" name="new-DependsOn" value="' . ($ARGS{'new-DependsOn'} || '' ). '" />',
- loc("Depended on by") => '<input size="10" name="DependsOn-new" value="' . ($ARGS{'DependsOn-new'} || '' ) . '" />',
- loc("Parents") => '<input size="10" name="new-MemberOf" value="' . ($ARGS{'new-MemberOf'} || '') . '" />',
- loc("Children") => '<input size="10" name="MemberOf-new" value="' . ($ARGS{'MemberOf-new'} || '') . '" />',
- loc("Refers to") => '<input size="10" name="new-RefersTo" value="' . ($ARGS{'new-RefersTo'} || '') . '" />',
- loc("Referred to by") => '<input size="10" name="RefersTo-new" value="' . ($ARGS{'RefersTo-new'} || ''). '" />'
+ loc("Depends on") => '<input size="10" name="new-DependsOn" value="' . $escape->($ARGS{'new-DependsOn'} || '' ). '" />',
+ loc("Depended on by") => '<input size="10" name="DependsOn-new" value="' . $escape->($ARGS{'DependsOn-new'} || '' ) . '" />',
+ loc("Parents") => '<input size="10" name="new-MemberOf" value="' . $escape->($ARGS{'new-MemberOf'} || '') . '" />',
+ loc("Children") => '<input size="10" name="MemberOf-new" value="' . $escape->($ARGS{'MemberOf-new'} || '') . '" />',
+ loc("Refers to") => '<input size="10" name="new-RefersTo" value="' . $escape->($ARGS{'new-RefersTo'} || '') . '" />',
+ loc("Referred to by") => '<input size="10" name="RefersTo-new" value="' . $escape->($ARGS{'RefersTo-new'} || ''). '" />'
);
</%perl>
commit c5f4ee6a1a64209629749602b54dfd2b6588d53e
Author: Shawn M Moore <sartak at bestpractical.com>
Date: Thu May 5 13:46:33 2011 -0400
Escape the name of the predefined search that was not found
diff --git a/share/html/m/tickets/search b/share/html/m/tickets/search
index a82763a..be7fd4a 100644
--- a/share/html/m/tickets/search
+++ b/share/html/m/tickets/search
@@ -78,7 +78,7 @@ my $search;
if ( $custom->Description eq $name ) { $search = $custom; last }
}
unless ( $search && $search->id ) {
- $m->out("Predefined search $name not found");
+ $m->out("Predefined search ".$m->interp->apply_escapes($name, 'h')." not found");
return;
}
}
commit 006cfcd255cf190a9fd71a9a9a959fe7ae50881c
Author: Shawn M Moore <sartak at bestpractical.com>
Date: Thu May 5 14:02:21 2011 -0400
Escape save search names when we report errors about loading them
diff --git a/share/html/Elements/ShowSearch b/share/html/Elements/ShowSearch
index cd6d6a8..c89d559 100644
--- a/share/html/Elements/ShowSearch
+++ b/share/html/Elements/ShowSearch
@@ -64,12 +64,12 @@ my $query_link_url = RT->Config->Get('WebPath').'/Search/Results.html';
if ($SavedSearch) {
my ( $container_object, $search_id ) = _parse_saved_search($SavedSearch);
unless ( $container_object ) {
- $m->out(loc("Either you have no rights to view saved search [_1] or identifier is incorrect", $SavedSearch));
+ $m->out(loc("Either you have no rights to view saved search [_1] or identifier is incorrect", $m->interp->apply_escapes($SavedSearch, 'h')));
return;
}
$search = $container_object->Attributes->WithId($search_id);
unless ( $search->Id && ref( $SearchArg = $search->Content ) eq 'HASH' ) {
- $m->out(loc("Saved Search [_1] not found", $SavedSearch)) unless $IgnoreMissing;
+ $m->out(loc("Saved Search [_1] not found", $m->interp->apply_escapes($SavedSearch, 'h'))) unless $IgnoreMissing;
return;
}
$SearchArg->{'SavedSearchId'} ||= $SavedSearch;
@@ -93,7 +93,7 @@ if ($SavedSearch) {
if ($custom->Description eq $Name) { $search = $custom; last }
}
unless ($search && $search->id) {
- $m->out("Predefined search $Name not found");
+ $m->out("Predefined search ".$m->interp->apply_escapes($Name, 'h')." not found");
return;
}
}
commit 3141f16f93e48a0f939319d8eaf8c1411562960a
Author: Shawn M Moore <sartak at bestpractical.com>
Date: Thu May 5 14:23:24 2011 -0400
Explicitly pass the type of escaping we want to apply_escapes
diff --git a/share/html/Admin/Tools/Shredder/Elements/Error/NoStorage b/share/html/Admin/Tools/Shredder/Elements/Error/NoStorage
index ea6e0b9..cbf3d1d 100644
--- a/share/html/Admin/Tools/Shredder/Elements/Error/NoStorage
+++ b/share/html/Admin/Tools/Shredder/Elements/Error/NoStorage
@@ -51,5 +51,5 @@ $Path => ''
<& /Admin/Elements/Header, Title => 'Error' &>
<& /Elements/Tabs &>
<div class="error">
-<% loc('Shredder needs a directory to write dumps to. Please check that you have <span class="file-path">[_1]</span> and it is writable by your web server.', $m->interp->apply_escapes( $Path ) ) |n%>
+<% loc('Shredder needs a directory to write dumps to. Please check that you have <span class="file-path">[_1]</span> and it is writable by your web server.', $m->interp->apply_escapes( $Path, 'h' ) ) |n%>
</div>
diff --git a/share/html/index.html b/share/html/index.html
index cac1d3c..be2157b 100755
--- a/share/html/index.html
+++ b/share/html/index.html
@@ -131,7 +131,7 @@ if ( $ARGS{'QuickCreate'} ) {
if ( $ARGS{'q'} ) {
- RT::Interface::Web::Redirect(RT->Config->Get('WebURL')."Search/Simple.html?q=".$m->interp->apply_escapes($ARGS{q}));
+ RT::Interface::Web::Redirect(RT->Config->Get('WebURL')."Search/Simple.html?q=".$m->interp->apply_escapes($ARGS{q}, 'u'));
}
</%init>
commit c36e510f788d72245d0464026fe22b1489b5c1f4
Author: Shawn M Moore <sartak at bestpractical.com>
Date: Thu May 5 14:32:23 2011 -0400
Use loc for interpolation
diff --git a/share/html/Elements/ShowSearch b/share/html/Elements/ShowSearch
index c89d559..1b37414 100644
--- a/share/html/Elements/ShowSearch
+++ b/share/html/Elements/ShowSearch
@@ -93,7 +93,7 @@ if ($SavedSearch) {
if ($custom->Description eq $Name) { $search = $custom; last }
}
unless ($search && $search->id) {
- $m->out("Predefined search ".$m->interp->apply_escapes($Name, 'h')." not found");
+ $m->out(loc("Predefined search [_1] not found", $m->interp->apply_escapes($Name, 'h')));
return;
}
}
diff --git a/share/html/m/tickets/search b/share/html/m/tickets/search
index be7fd4a..ae538be 100644
--- a/share/html/m/tickets/search
+++ b/share/html/m/tickets/search
@@ -78,7 +78,7 @@ my $search;
if ( $custom->Description eq $name ) { $search = $custom; last }
}
unless ( $search && $search->id ) {
- $m->out("Predefined search ".$m->interp->apply_escapes($name, 'h')." not found");
+ $m->out(loc("Predefined search [_1] not found", $m->interp->apply_escapes($name, 'h')));
return;
}
}
commit c29107c60454340eaac64f00acacce8b76bc1970
Author: Thomas Sibley <trs at bestpractical.com>
Date: Mon Nov 14 19:00:35 2011 -0500
Iterate attachments as the creator of the current transaction when sending mail
Check CurrentUserCanSee before trying to add an attachment since it
could otherwise end up empty now that we have the correct current user.
Additionally, simply check RT::Transaction's CurrentUserCanSee when
iterating inside an RT::Attachments object rather than maintaining a
different but similar conditional tree. CurrentUserCanSee correctly
access checks transaction types like EmailRecord, for example.
This resolves part of CVE-2011-2084.
diff --git a/lib/RT/Action/SendEmail.pm b/lib/RT/Action/SendEmail.pm
index 553b736..0105373 100644
--- a/lib/RT/Action/SendEmail.pm
+++ b/lib/RT/Action/SendEmail.pm
@@ -348,7 +348,7 @@ sub AddAttachments {
$MIMEObj->head->delete('RT-Attach-Message');
- my $attachments = RT::Attachments->new(RT->SystemUser);
+ my $attachments = RT::Attachments->new( $self->TransactionObj->CreatorObj );
$attachments->Limit(
FIELD => 'TransactionId',
VALUE => $self->TransactionObj->Id
@@ -408,6 +408,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,
@@ -466,8 +470,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( $self->TransactionObj->CreatorObj );
my $txn_alias = $attachs->TransactionAlias;
$attachs->Limit( ALIAS => $txn_alias, FIELD => 'Type', VALUE => 'Create' );
$attachs->Limit(
diff --git a/lib/RT/Attachments.pm b/lib/RT/Attachments.pm
index c02c458..15d6d6b 100644
--- a/lib/RT/Attachments.pm
+++ b/lib/RT/Attachments.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;
}
commit c8466ec04ffaad0658f65cb104cde2c2a11bb499
Author: Thomas Sibley <trs at bestpractical.com>
Date: Wed Nov 16 12:11:18 2011 -0500
Ensure the empty CFVs collection never returns results after a failed rights check
This is identical to negative limiting we do elsewhere to ensure no
records are returned.
This resolves part of CVE-2011-2084.
diff --git a/lib/RT/CustomField.pm b/lib/RT/CustomField.pm
index 820f6ee..e52605b 100644
--- a/lib/RT/CustomField.pm
+++ b/lib/RT/CustomField.pm
@@ -526,6 +526,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 );
}
return ($cf_values);
}
commit 4f0c2bd4c1a75d166937984dc2fa42bebdcf46aa
Author: Thomas Sibley <trs at bestpractical.com>
Date: Wed Nov 16 12:52:02 2011 -0500
Push id = 0 limits into an ACL subclause
This ensures the id = 0 condition isn't OR'd with a subsequent limit
after we return the collection.
diff --git a/lib/RT/CustomField.pm b/lib/RT/CustomField.pm
index e52605b..c5436da 100644
--- a/lib/RT/CustomField.pm
+++ b/lib/RT/CustomField.pm
@@ -527,7 +527,7 @@ sub Values {
if ( $self->id && $self->CurrentUserHasRight( 'SeeCustomField') ) {
$cf_values->LimitToCustomField( $self->Id );
} else {
- $cf_values->Limit( FIELD => 'id', VALUE => 0 );
+ $cf_values->Limit( FIELD => 'id', VALUE => 0, SUBCLAUSE => 'acl' );
}
return ($cf_values);
}
diff --git a/lib/RT/Ticket.pm b/lib/RT/Ticket.pm
index 316fbff..954015d 100644
--- a/lib/RT/Ticket.pm
+++ b/lib/RT/Ticket.pm
@@ -2357,7 +2357,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;
}
diff --git a/lib/RT/Transaction.pm b/lib/RT/Transaction.pm
index 94e46e3..04a74f2 100644
--- a/lib/RT/Transaction.pm
+++ b/lib/RT/Transaction.pm
@@ -506,7 +506,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'};
}
commit dbd78716780b22733e92e8048691e560a31b8494
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Thu Nov 17 17:23:44 2011 -0500
Ensure that publicly cachable content does not contain Set-Cookie headers
diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index c880115..a716d7c 100644
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -801,6 +801,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;
commit c86408bba0f166786e0c48bb5e7be5126cf1039a
Author: Thomas Sibley <trs at bestpractical.com>
Date: Wed Nov 16 14:19:19 2011 -0500
Only run known formatters in RT::Date
To make a formatter known to RT, you should:
push @RT::Date::FORMATTERS, 'YourFormatter';
This resolves part of CVE-2011-4458.
diff --git a/lib/RT/Date.pm b/lib/RT/Date.pm
index ceb1c21..d6283bd 100644
--- a/lib/RT/Date.pm
+++ b/lib/RT/Date.pm
@@ -545,6 +545,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 );
}
@@ -583,6 +587,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
commit 303affec335d0e44bcb374ebd5cb6af862d013f7
Author: Thomas Sibley <trs at bestpractical.com>
Date: Fri Nov 18 18:47:07 2011 -0500
Don't execute non-Perl templates in RT::Action::CreateTickets
Simple templates passed to the CreateTickets action used to be evaluated
directly with Text::Template, including any Perl contained within them.
This allowed users without ExecuteCode—or systems configured to
DisallowExecuteCode—to still create templates and scrips that executed
Perl code.
This resolves part of CVE-2011-4458.
diff --git a/lib/RT/Action/CreateTickets.pm b/lib/RT/Action/CreateTickets.pm
index 32b2bc0..264a772 100644
--- a/lib/RT/Action/CreateTickets.pm
+++ b/lib/RT/Action/CreateTickets.pm
@@ -322,9 +322,19 @@ sub Prepare {
}
+ my $active = 0;
+ if ( $self->TemplateObj->Type eq 'Perl' ) {
+ $active = 1;
+ } else {
+ RT->Logger->info(sprintf(
+ "Template #%d is type %s. You most likely want to use a Perl template instead.",
+ $self->TemplateObj->id, $self->TemplateObj->Type
+ ));
+ }
+
$self->Parse(
Content => $self->TemplateObj->Content,
- _ActiveContent => 1
+ _ActiveContent => $active,
);
return 1;
commit 5f265b6e7a59e60c6317985b9aacafc0bbd54f66
Author: Thomas Sibley <trs at bestpractical.com>
Date: Wed Jan 4 14:51:26 2012 -0500
Prevent user-controlled partial component paths from walking up directories
Mason didn't let through paths outside of it's component roots, but it
did mean private components were accessible semi-directly.
diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index 2c5123c..d1be557 100644
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -822,6 +822,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
diff --git a/lib/RT/User.pm b/lib/RT/User.pm
index cbc1a8f..09d90bf 100644
--- a/lib/RT/User.pm
+++ b/lib/RT/User.pm
@@ -1331,12 +1331,13 @@ sub Stylesheet {
my $style = RT->Config->Get('WebDefaultStylesheet', $self->CurrentUser);
+ if (RT::Interface::Web->ComponentPathIsSafe($style)) {
+ my @css_paths = map { $_ . '/NoAuth/css' } RT::Interface::Web->ComponentRoots;
- my @css_paths = map { $_ . '/NoAuth/css' } RT::Interface::Web->ComponentRoots;
-
- for my $css_path (@css_paths) {
- if (-d "$css_path/$style") {
- return $style
+ for my $css_path (@css_paths) {
+ if (-d "$css_path/$style") {
+ return $style
+ }
}
}
diff --git a/share/html/Elements/ShowUser b/share/html/Elements/ShowUser
index 8b21220..7b31faa 100644
--- a/share/html/Elements/ShowUser
+++ b/share/html/Elements/ShowUser
@@ -51,7 +51,7 @@
# $Address is Email::Address object
my $comp = '/Elements/ShowUser'. ucfirst lc $style;
-unless ( $m->comp_exists( $comp ) ) {
+unless ( RT::Interface::Web->ComponentPathIsSafe($comp) and $m->comp_exists( $comp ) ) {
$RT::Logger->error(
'Either system config or user #'
. $session{'CurrentUser'}->id
diff --git a/share/html/Helpers/Toggle/ShowRequestor b/share/html/Helpers/Toggle/ShowRequestor
index 7da4001..5fa77e9 100644
--- a/share/html/Helpers/Toggle/ShowRequestor
+++ b/share/html/Helpers/Toggle/ShowRequestor
@@ -47,7 +47,9 @@
%# END BPS TAGGED BLOCK }}}
<%INIT>
my $TicketTemplate = "/Ticket/Elements/ShowRequestorTickets$Status";
-$TicketTemplate = "/Ticket/Elements/ShowRequestorTicketsActive" unless $m->comp_exists($TicketTemplate);
+$TicketTemplate = "/Ticket/Elements/ShowRequestorTicketsActive"
+ unless RT::Interface::Web->ComponentPathIsSafe($TicketTemplate)
+ and $m->comp_exists($TicketTemplate);
my $user_obj = RT::User->new($session{CurrentUser});
my ($val, $msg) = $user_obj->Load($Requestor);
unless ($val) {
diff --git a/share/html/Ticket/Elements/ShowRequestor b/share/html/Ticket/Elements/ShowRequestor
index 07fa6ec..0f3cd39 100755
--- a/share/html/Ticket/Elements/ShowRequestor
+++ b/share/html/Ticket/Elements/ShowRequestor
@@ -175,7 +175,9 @@ unless ( $DefaultTicketsTab eq 'None' ) {
}
my $TicketTemplate = "ShowRequestorTickets$DefaultTicketsTab";
-$TicketTemplate = "ShowRequestorTicketsActive" unless $m->comp_exists($TicketTemplate);
+$TicketTemplate = "ShowRequestorTicketsActive"
+ unless RT::Interface::Web->ComponentPathIsSafe($TicketTemplate)
+ and $m->comp_exists($TicketTemplate);
</%INIT>
<%ARGS>
$Ticket=>undef
commit 0aaecd1166d8ed3aef066fa833eaa974190bec42
Author: Thomas Sibley <trs at bestpractical.com>
Date: Wed Jan 4 16:51:05 2012 -0500
Make CheckIntegrity idempotent on a running install
Avoid reconnecting to the database if we have a handle and stop trying
to initialize the logger. These changes make it safe to call as many
times as you'd like on a running instance without affecting the app.
Specifically, CheckIntegrity is now safely callable from places other
than server initialization.
Note that any calls to CheckIntegrity must now come _after_
InitLogging() is called. In RT 4, CheckIntegrity is only called in
rt-server.
diff --git a/lib/RT/Handle.pm b/lib/RT/Handle.pm
index 308f5ba..73c99de 100644
--- a/lib/RT/Handle.pm
+++ b/lib/RT/Handle.pm
@@ -226,14 +226,12 @@ sub CheckIntegrity {
my $self = shift;
$self = new $self unless ref $self;
- do {
+ unless ($RT::Handle and $RT::Handle->dbh) {
local $@;
unless ( eval { RT::ConnectToDatabase(); 1 } ) {
return (0, 'no connection', "$@");
}
- };
-
- RT::InitLogging();
+ }
require RT::CurrentUser;
my $test_user = RT::CurrentUser->new;
diff --git a/sbin/rt-server.in b/sbin/rt-server.in
index 022559b..f74bcdb 100755
--- a/sbin/rt-server.in
+++ b/sbin/rt-server.in
@@ -91,6 +91,7 @@ if (grep { m/help/ } @ARGV) {
require RT;
RT->LoadConfig();
+RT->InitLogging();
require Module::Refresh if RT->Config->Get('DevelMode');
require RT::Handle;
commit a3c69912e79951f1ef1b2df527f86d0f7ee4ca8b
Author: Thomas Sibley <trs at bestpractical.com>
Date: Wed Jan 4 16:56:59 2012 -0500
Refuse to turn on InstallMode when we have database integrity
This prevents install mode from activating on a working install and
resolves part of CVE-2011-4458.
diff --git a/lib/RT.pm b/lib/RT.pm
index 5e63a00..36436d1 100644
--- a/lib/RT.pm
+++ b/lib/RT.pm
@@ -678,11 +678,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;
}
commit fa17a99b427c1c0a627bd144d692633b078bb1cb
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Thu Jan 5 22:40:38 2012 -0500
Prevent actual error messages from propagating to the user
With DevelMode off, Mason formats runtime errors using the 'brief'
format, which includes the full path on disk to where the error ocurred.
This is a potential information leak. Instead, provide a generic error
message to the user, but log the actual error in the logs.
diff --git a/lib/RT/Interface/Web/Handler.pm b/lib/RT/Interface/Web/Handler.pm
index b5f9f53..3f8c02a 100644
--- a/lib/RT/Interface/Web/Handler.pm
+++ b/lib/RT/Interface/Web/Handler.pm
@@ -74,7 +74,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,
) };
@@ -202,6 +202,13 @@ sub CleanupRequest {
}
+sub HTML::Mason::Exception::as_rt_error {
+ my ($self) = @_;
+ $RT::Logger->error( $self->full_message );
+ return "An internal RT error has occurred.";
+}
+
+
# PSGI App
use RT::Interface::Web::Handler;
commit 55cb6f4032cd9a98ee650ab88515b2b8c5b09634
Author: Thomas Sibley <trs at bestpractical.com>
Date: Fri Jan 6 11:22:40 2012 -0500
Escape backslashes in text used for GraphViz input
Previously you could avoid the double quote escaping by proceeding your
double quote with a backslash. The backslash naively added by gv_escape
would itself be escaped by your original backslash, leaving the quote
unescaped. This most often simply broke GraphViz output rendering it
unusable.
While injecting arbitrary input into GraphViz is of unlikely utility,
GraphViz did have a buffer overflow vulnerability in versions 2.20.2 and
earlier as described in CVE-2008-4555.
diff --git a/lib/RT/Graph/Tickets.pm b/lib/RT/Graph/Tickets.pm
index 740e372..1419726 100644
--- a/lib/RT/Graph/Tickets.pm
+++ b/lib/RT/Graph/Tickets.pm
@@ -100,7 +100,7 @@ EOT
sub gv_escape($) {
my $value = shift;
- $value =~ s{(?=")}{\\}g;
+ $value =~ s{(?=["\\])}{\\}g;
return $value;
}
commit 0d10462c93c0369a7c973f83b82893ec2b78af30
Author: Thomas Sibley <trs at bestpractical.com>
Date: Fri Jan 6 14:06:36 2012 -0500
Inherit from the normal autohandler chain when serving Shredder backups
The normal inheritance chain of:
/autohandler
/Admin/autohandler
/Admin/Tools/Shredder/autohandler
provides appropriate access control at each level. By inheriting from
nothing, the shredder dump dhandler was called directly, making a
complete end-run around all ACLs. Anyone who could guess (i.e. brute
force) the very predictable dump filenames would be treated to a SQL
dump of shredded data, no login required.
Instead of inheriting from nothing to avoid the footer, simply abort the
request at the end of the dhandler.
Fixes part of CVE-2011-2084.
diff --git a/share/html/Admin/Tools/Shredder/Dumps/dhandler b/share/html/Admin/Tools/Shredder/Dumps/dhandler
index e742001..53b8065 100644
--- a/share/html/Admin/Tools/Shredder/Dumps/dhandler
+++ b/share/html/Admin/Tools/Shredder/Dumps/dhandler
@@ -48,9 +48,6 @@
<%ATTR>
AutoFlush => 0
</%ATTR>
-<%FLAGS>
-inherit => undef
-</%FLAGS>
<%INIT>
my $arg = $m->dhandler_arg;
$m->abort(404) if $arg =~ m{\.\.|/|\\};
@@ -64,5 +61,5 @@ my $buf;
while( read $fh, $buf, 1024*1024 ) {
$m->out($buf);
}
-return 0;
+$m->abort;
</%INIT>
commit 8ebe790ea3271c7fedbc9fb6357aaa1f80b169ef
Author: Thomas Sibley <trs at bestpractical.com>
Date: Fri Jan 6 16:56:17 2012 -0500
Refactor HTML scrubbing to make it easier to customize what is allowed
This is in preparation for stricter rules that may want slight loosening
for some installs (i.e. a regex rather than flat out denial).
diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index 2c5123c..155184b 100644
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -2784,50 +2784,68 @@ sub ScrubHTML {
=head2 _NewScrubber
-Returns a new L<HTML::Scrubber> object. Override this if you insist on
-letting more HTML through.
+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 = (
+ id => 1,
+ class => 1,
+ # 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,
+);
+
sub _NewScrubber {
require HTML::Scrubber;
my $scrubber = HTML::Scrubber->new();
$scrubber->default(
0,
{
- '*' => 0,
- id => 1,
- class => 1,
- # 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,
- }
+ %SCRUBBER_ALLOWED_ATTRIBUTES,
+ '*' => 0, # require attributes be explicitly allowed
+ },
);
$scrubber->deny(qw[*]);
- $scrubber->allow(
- 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]
- );
+ $scrubber->allow(@SCRUBBER_ALLOWED_TAGS);
+
+ # 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;
commit de58d4d2cf5e8742cd8ee3784f50923a19b338ae
Author: Thomas Sibley <trs at bestpractical.com>
Date: Fri Jan 6 17:28:34 2012 -0500
Add a way to specify tag-specific attribute rules for scrubbing
Currently unused, but immensely useful.
diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index 155184b..3ac4e14 100644
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -2831,6 +2831,8 @@ our %SCRUBBER_ALLOWED_ATTRIBUTES = (
}ix,
);
+our %SCRUBBER_RULES = ();
+
sub _NewScrubber {
require HTML::Scrubber;
my $scrubber = HTML::Scrubber->new();
@@ -2843,6 +2845,7 @@ sub _NewScrubber {
);
$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.
commit 29f7442f16352369779a43ad39a02149470032cd
Author: Thomas Sibley <trs at bestpractical.com>
Date: Fri Jan 6 17:46:25 2012 -0500
Scrub class and id attributes from HTML instead of passing them through
A large part of RT's layout and enhancement with Javascript depends on
classes and ids. Passing through those attributes means ticket history
can trivially piggyback on RT's core CSS and JS to do malicious things
like hide content or style and move fake elements on top of core UI.
Since classes and ids are useful, however, for site-specific RT
customizations around correspondence, the scrubbing configuration was
refactored in previously commits to make it easier to tweak what tags
and attributes are allowed.
Resolves part of CVE-2011-2083.
diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index 3ac4e14..e1472d5 100644
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -2803,8 +2803,6 @@ our @SCRUBBER_ALLOWED_TAGS = qw(
);
our %SCRUBBER_ALLOWED_ATTRIBUTES = (
- id => 1,
- class => 1,
# 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,
commit 5506d7cd5646ef95bb94ce9a1585aa69e14539e1
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Fri Jan 6 20:24:07 2012 -0500
/Articles/Topics.html uses id= for topic ids, not article ids
This avoids critical warnings in the logs about not being able to check
rights on non-existant Article objects.
diff --git a/share/html/Elements/Tabs b/share/html/Elements/Tabs
index 953e938..afd84fb 100755
--- a/share/html/Elements/Tabs
+++ b/share/html/Elements/Tabs
@@ -788,7 +788,7 @@ my $build_main_nav = sub {
my $tabs = PageMenu();
$tabs->child( search => title => loc("Search"), path => "/Articles/Article/Search.html" );
$tabs->child( create => title => loc("New Article" ), path => "/Articles/Article/PreCreate.html" );
- if ( ( $m->request_args->{'id'} || '' ) =~ /^(\d+)$/ ) {
+ if ( $request_path =~ m{^/Articles/Article/} and ( $m->request_args->{'id'} || '' ) =~ /^(\d+)$/ ) {
my $id = $1;
my $obj = RT::Article->new( $session{'CurrentUser'} );
$obj->Load($id);
commit b7393fb869e3ee843389e932e07a59266c4ce2a6
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Fri Jan 6 20:26:19 2012 -0500
Rework topic display to not make use of $m->print everywhere
This closes a number of XSS vulnerabilities concerning topic names.
This commit partially resolves CVE-2011-2083.
diff --git a/share/html/Articles/Article/Elements/EditTopics b/share/html/Articles/Article/Elements/EditTopics
index ed3ef47..770c500 100644
--- a/share/html/Articles/Article/Elements/EditTopics
+++ b/share/html/Articles/Article/Elements/EditTopics
@@ -47,35 +47,32 @@
%# END BPS TAGGED BLOCK }}}
<input type="hidden" name="EditTopics" value="1" />
<select multiple size="10" name="Topics">
-<%perl>
-if (@Classes) {
- $m->print("<optgroup label=\"Current classes (".join (' ',map {$_->Name} @Classes).")\">")
- unless $OnlyThisClass;
- $inTree->traverse(sub {
- my $tree = shift;
- my $topic = $tree->getNodeValue;
- $m->print("<option value=\"".$topic->Id."\""
- .(exists $topics{$topic->Id} ? " selected" : "").">"
- .(" " x ($tree->getDepth*5)).($topic->Name || loc("(no name)"))."</option>\n");
- });
-}
-unless ($OnlyThisClass) {
- my $class = $Classes[-1]->Id;
- $otherTree->traverse(sub {
- my $tree = shift;
- my $topic = $tree->getNodeValue;
- unless ($topic->ObjectId == $class) {
- $class = $topic->ObjectId;
- $m->print("</optgroup>\n");
- my $c = RT::Class->new($session{'CurrentUser'});
- $c->Load($topic->ObjectId);
- $m->print("<optgroup label=\"".$c->Name."\">\n");
- }
- $m->print("<option value=\"".$topic->Id."\""
- .(exists $topics{$topic->Id} ? " selected" : "").">"
- .(" " x ($tree->getDepth*5)).($topic->Name || loc("(no name)"))."</option>\n");
- });
-</%perl>
+% if (@Classes) {
+% unless ($OnlyThisClass) {
+<optgroup label="Current classes (<% join(" ", map {$_->Name} @Classes) %>)">
+% }
+% $inTree->traverse(sub {
+% my $tree = shift;
+% my $topic = $tree->getNodeValue;
+<option value="<% $topic->Id %>" <% exists $topics{$topic->Id} ? "selected" : "" %> >\
+<% " " x ($tree->getDepth*5) |n %><% $topic->Name || loc("(no name)") %></option>
+% });
+% }
+% unless ($OnlyThisClass) {
+% my $class = $Classes[-1]->Id;
+% $otherTree->traverse(sub {
+% my $tree = shift;
+% my $topic = $tree->getNodeValue;
+% unless ($topic->ObjectId == $class) {
+% $class = $topic->ObjectId;
+</optgroup>
+% my $c = RT::Class->new($session{'CurrentUser'});
+% $c->Load($topic->ObjectId);
+<optgroup label="<% $c->Name %>">
+% }
+<option value="<% $topic->Id %>" <% exists $topics{$topic->Id} ? "selected" : "" %> >\
+<% " " x ($tree->getDepth*5) |n %><% $topic->Name || loc("(no name)") %></option>
+% });
</optgroup>
% }
</select>
diff --git a/share/html/Articles/Elements/ShowTopicLink b/share/html/Articles/Elements/ShowTopicLink
new file mode 100644
index 0000000..7b6d550
--- /dev/null
+++ b/share/html/Articles/Elements/ShowTopicLink
@@ -0,0 +1,27 @@
+<%args>
+$Topic
+$Class => 0
+</%args>
+% if ($Link) {
+<a href="Topics.html?id=<% $Topic->Id %>&class=<% $Class %>">\
+% }
+<% $Topic->Name() || loc("(no name)") %>\
+% if ($Topic->Description) {
+: <% $Topic->Description %>
+% }
+
+% if ( $Articles->Count ) {
+ (<&|/l, $Articles->Count &>[quant,_1,article]</&>)
+% }
+
+% if ($Link) {
+</a>
+% }
+
+<%init>
+my $Articles = RT::ObjectTopics->new( $session{'CurrentUser'} );
+$Articles->Limit( FIELD => 'ObjectType', VALUE => 'RT::Article' );
+$Articles->Limit( FIELD => 'Topic', VALUE => $Topic->Id );
+
+my $Link = $Topic->Children->Count || $Articles->Count;
+</%init>
diff --git a/share/html/Articles/Topics.html b/share/html/Articles/Topics.html
index 27b2eed..52ecc0a 100644
--- a/share/html/Articles/Topics.html
+++ b/share/html/Articles/Topics.html
@@ -48,7 +48,6 @@
<& /Elements/Header, Title => loc('Browse by topic') &>
<& /Elements/Tabs &>
-<& /Elements/ListActions, actions => \@Actions &>
<a href="Topics.html"><&|/l&>All topics</&></a>
% if (defined $class) {
> <a href="Topics.html?class=<%$currclass_id%>"><% $currclass_name %></a>
@@ -59,71 +58,41 @@
% }
<br />
<h1><&|/l&>Browse by topic</&></h1>
-<%perl>
-if (defined $class) {
- $m->print('<h2>'.'<a href="'.
- RT->Config->Get('WebPath')."/Articles/Topics.html?class=" . $currclass_id
- .'">'.$currclass_name."</a></h2>\n");
- ProduceTree(\@Actions, $currclass, $currclass_id, $currclass_name, 0, $id);
-} else {
- $m->print("<ul>\n");
- while (my $c = $Classes->Next) {
- $m->print('<li><h2>'.'<a href="'.
- RT->Config->Get('WebPath')."/Articles/Topics.html?class=" . $c->Id
- .'">'.$c->Name."</a></h2>\n");
- $m->print("\n</li>\n");
- }
- $m->print(qq|<li><h2><a href="|.RT->Config->Get('WebPath').qq|/Articles/Topics.html?class=0">|.loc('Global Topics').qq|</a></h2></li>\n|);
- $m->print("</ul>\n");
-}
-</%perl>
+% if (defined $class) {
+<h2><a href="<% RT->Config->Get('WebPath') %>/Articles/Topics.html?class=<% $currclass_id %>"><% $currclass_name %></a></h2>
+% my $rtopic = RT::Topic->new( $session{'CurrentUser'} );
+% $rtopic->Load($id);
+% unless ( $rtopic->Id()
+% && $rtopic->ObjectId() == $currclass->Id )
+% {
+% # Show all of them
+% $ProduceTree->( 0 );
+% } else {
+% my @showtopics = ( $rtopic );
+% my $parent = $rtopic->ParentObj;
+% while ( $parent->Id ) {
+% unshift @showtopics, $parent;
+% $parent = $parent->ParentObj;
+% }
+% # List the topics.
+% for my $t ( @showtopics ) {
+<ul><li><& /Articles/Elements/ShowTopicLink, Topic => $t, Class => $currclass_id &>
+% $ProduceTree->( $id ) if $t->Id == $id;
+% }
+% for ( @showtopics ) {
+ </li></ul>
+% }
+% }
+% } else {
+<ul>
+% while (my $c = $Classes->Next) {
+<li><h2><a href="<% RT->Config->Get('WebPath') %>/Articles/Topics.html?class=<% $c->Id %>"><% $c->Name %></a></h2></li>
+% }
+<li><h2><a href="<% RT->Config->Get('WebPath') %>/Articles/Topics.html?class=0"><&|/l&>Global Topics</&></a></h2></li>
+</ul>
+% }
<br />
-<%perl>
-my @articles;
-if ($id or $showall) {
- my $Articles = RT::ObjectTopics->new($session{'CurrentUser'});
- $Articles->Limit(FIELD => 'ObjectType', VALUE => 'RT::Article');
- if ($id) {
- $Articles->Limit(FIELD => 'Topic', VALUE => $id, ENTRYAGGREGATOR => 'OR');
- if ($showall) {
- my $kids = $currtopic->Children;
- while (my $k = $kids->Next) {
- $Articles->Limit(FIELD => 'Topic', VALUE => $k->Id,
- ENTRYAGGREGATOR => 'OR');
- }
- }
- }
- @articles = map {$a = RT::Article->new($session{'CurrentUser'}); $a->Load($_->ObjectId); $a} @{$Articles->ItemsArrayRef}
-} elsif ($class) {
- my $Articles = RT::Articles->new($session{'CurrentUser'});
- my $TopicsAlias = $Articles->Join(
- TYPE => 'left',
- ALIAS1 => 'main',
- FIELD1 => 'id',
- TABLE2 => 'ObjectTopics',
- FIELD2 => 'ObjectId',
- );
- $Articles->Limit(
- LEFTJOIN => $TopicsAlias,
- FIELD => 'ObjectType',
- VALUE => 'RT::Article',
- );
- $Articles->Limit(
- ALIAS => $TopicsAlias,
- FIELD => 'Topic',
- OPERATOR => 'IS',
- VALUE => 'NULL',
- QUOTEVALUE => 0,
- );
- $Articles->Limit(
- FIELD => 'Class',
- OPERATOR => '=',
- VALUE => $class,
- );
- @articles = @{$Articles->ItemsArrayRef};
-}
-</%perl>
% if (@articles) {
% if ($id) {
@@ -139,7 +108,6 @@ if ($id or $showall) {
% }
<%init>
-my @Actions;
my $Classes;
my $currclass;
my $currclass_id;
@@ -167,106 +135,65 @@ if ($id) {
$currtopic->Load($id);
}
-# A subroutine that iterates through topics and their children, producing
-# the necessary ul, li, and href links for the table of contents. Thank
-# heaven for query caching. The $restrict variable is used to display only
-# the branch of the hierarchy which contains that topic ID.
-
-sub ProduceTree {
- my ( $Actions, $currclass, $currclass_id, $currclass_name, $parentid, $restrictid ) = @_;
- $parentid = 0 unless $parentid;
-
- # Deal with tree restriction, if any.
- if ($restrictid) {
- my $rtopic = RT::Topic->new( $session{'CurrentUser'} );
- $rtopic->Load($restrictid);
- unless ( $rtopic->Id()
- && $rtopic->ObjectId() == $currclass_id )
- {
- push( @{$Actions},"Could not restrict view to topic $restrictid");
-
- # Start over, without the restriction.
- &ProduceTree( $Actions, $currclass, $currclass_id, $currclass_name, $parentid, undef );
- } else {
- my @showtopics;
- push( @showtopics, $rtopic );
- my $parent = $rtopic->ParentObj;
- while ( $parent->Id ) {
- push( @showtopics, $parent );
- my $newparent = $parent->ParentObj;
- $parent = $newparent;
- }
-
- # List the topics.
- my $indents = @showtopics;
- while ( my $t = pop @showtopics ) {
- print "<ul>";
- print &MakeLinks( $t, $currclass, $currclass_id, $currclass_name, $t->Children->Count );
- if ( $t->Id == $restrictid ) {
- &ProduceTree( $Actions, $currclass, $currclass_id, $currclass_name, $restrictid, undef );
- }
- }
- print "</ul>" x $indents;
- }
- } else {
-
- # No restriction in place. Build the entire tree.
- my $topics = RT::Topics->new( $session{'CurrentUser'} );
- $topics->LimitToObject($currclass);
- $topics->LimitToKids($parentid);
- $topics->OrderBy( FIELD => 'Name' );
- print "<ul>" if $topics->Count;
- while ( my $t = $topics->Next ) {
- if ( $t->Children->Count ) {
- print &MakeLinks( $t, $currclass, $currclass_id, $currclass_name, 1 );
- &ProduceTree( $Actions, $currclass, $currclass_id, $currclass_name, $t->Id );
- } else {
- print &MakeLinks( $t, $currclass, $currclass_id, $currclass_name, 0 );
- }
- }
- print "</ul>\n" if $topics->Count;
+my $ProduceTree;
+$ProduceTree = sub {
+ my ( $parentid ) = @_;
+ my $topics = RT::Topics->new( $session{'CurrentUser'} );
+ $topics->LimitToObject($currclass);
+ $topics->LimitToKids($parentid || 0);
+ $topics->OrderBy( FIELD => 'Name' );
+ return unless $topics->Count;
+ $m->out("<ul>");
+ while ( my $t = $topics->Next ) {
+ $m->out("<li>");
+ $m->comp("/Articles/Elements/ShowTopicLink",
+ Topic => $t,
+ Class => $currclass_id,
+ );
+ $ProduceTree->( $t->Id ) if $t->Children->Count;
+ $m->out("</li>");
}
-}
-
-sub MakeLinks {
- my ( $topic, $currclass, $currclass_id, $currclass_name, $haschild ) = @_;
- my $query;
- my $output;
-
- if ( ref($topic) eq 'RT::Topic' ) {
-
- my $topic_info = $topic->Name() || loc("(no name)");
- $topic_info .= ": " . $topic->Description() if $topic->Description;
-
- if ($haschild) { # has topics below it
- $query = "Topics.html?id=" . $topic->Id . "&class=" . $currclass_id;
- $output = qq(<li><a href="$query">$topic_info</a>);
- } else {
- $output = qq(<li>$topic_info);
- }
+ $m->out("</ul>");
+};
- my $Articles = RT::ObjectTopics->new( $session{'CurrentUser'} );
- $Articles->Limit( FIELD => 'ObjectType', VALUE => 'RT::Article' );
- $Articles->Limit( FIELD => 'Topic', VALUE => $topic->Id );
- if ( $Articles->Count ) {
- my $article_text = " (" . loc( "[quant,_1,article]", $Articles->Count ) . ")";
- my $query = "Topics.html?id=" . $topic->Id . "&class=$currclass_id&showall=1";
- $output .= qq(<a href="$query">$article_text</a>);
- }
-
- $output .= "</li>\n";
-
- } else {
-
- # This builds a link for the class specified, with no particular topic.
- $query = "Topics.html?class=" . $currclass_id;
- $output = "<li><a href=\"$query\">" . $currclass_name . "</a>";
- $output .= ": " . $currclass->Description if $currclass->Description;
+my @articles;
+if ($id) {
+ my $Articles = RT::ObjectTopics->new($session{'CurrentUser'});
+ $Articles->Limit(FIELD => 'ObjectType', VALUE => 'RT::Article');
+ $Articles->Limit(FIELD => 'Topic', VALUE => $id);
+ while (my $objtopic = $Articles->Next) {
+ my $a = RT::Article->new($session{'CurrentUser'});
+ $a->Load($objtopic->ObjectId);
+ push @articles, $a;
}
-
- return $output;
+} elsif ($class) {
+ my $Articles = RT::Articles->new($session{'CurrentUser'});
+ my $TopicsAlias = $Articles->Join(
+ TYPE => 'left',
+ ALIAS1 => 'main',
+ FIELD1 => 'id',
+ TABLE2 => 'ObjectTopics',
+ FIELD2 => 'ObjectId',
+ );
+ $Articles->Limit(
+ LEFTJOIN => $TopicsAlias,
+ FIELD => 'ObjectType',
+ VALUE => 'RT::Article',
+ );
+ $Articles->Limit(
+ ALIAS => $TopicsAlias,
+ FIELD => 'Topic',
+ OPERATOR => 'IS',
+ VALUE => 'NULL',
+ QUOTEVALUE => 0,
+ );
+ $Articles->Limit(
+ FIELD => 'Class',
+ OPERATOR => '=',
+ VALUE => $class,
+ );
+ @articles = @{$Articles->ItemsArrayRef};
}
-
</%init>
<%args>
commit d1655ade198840f1cd33690ecf1ff2172181afd0
Author: Kevin Falcone <falcone at bestpractical.com>
Date: Mon Jan 9 13:41:52 2012 -0500
Encourage users to look in the logs when an error happens.
diff --git a/lib/RT/Interface/Web/Handler.pm b/lib/RT/Interface/Web/Handler.pm
index 3f8c02a..de0609c 100644
--- a/lib/RT/Interface/Web/Handler.pm
+++ b/lib/RT/Interface/Web/Handler.pm
@@ -205,7 +205,7 @@ sub CleanupRequest {
sub HTML::Mason::Exception::as_rt_error {
my ($self) = @_;
$RT::Logger->error( $self->full_message );
- return "An internal RT error has occurred.";
+ return "An internal RT error has occurred. Your administrator can find more details in RT's log files.";
}
commit 4faebb190e299f7b6698cf5a16fffc49d3c8ea8a
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Fri Feb 17 13:22:57 2012 -0500
Secure the bestpractical.com news portal request using HTTPS
On an SSL-protected instance of RT, the existance of an
http://bestpractical.com request from the Admin page opens up the
possibility of a hostile network employing a man-in-the-middle attack to
insert malicious javascript. Prevent this, by loading the news portal
via HTTPS rather than HTTP.
diff --git a/share/html/Admin/Elements/Portal b/share/html/Admin/Elements/Portal
index d5e75c5..821ed57 100644
--- a/share/html/Admin/Elements/Portal
+++ b/share/html/Admin/Elements/Portal
@@ -47,6 +47,6 @@
%# END BPS TAGGED BLOCK }}}
<div id="rt-portal">
<&| /Widgets/TitleBox, title => 'RT Portal' &>
-<iframe src="http://bestpractical.com/rt/integration/news?utm_source=rt&utm_medium=iframe&utm_campaign=<%$RT::VERSION%>"></iframe>
+<iframe src="https://bestpractical.com/rt/integration/news?utm_source=rt&utm_medium=iframe&utm_campaign=<%$RT::VERSION%>"></iframe>
</&>
</div>
commit bb35edd1aaa63499ff5e03f2b1747c9daa334f9f
Author: Kevin Falcone <falcone at bestpractical.com>
Date: Thu Mar 22 19:10:14 2012 -0400
We did not find and upgrade passwords for disabled users.
This script can be safely re-run on RT installations which were
previously upgraded.
diff --git a/etc/upgrade/vulnerable-passwords.in b/etc/upgrade/vulnerable-passwords.in
index 728786f..a3d719c 100755
--- a/etc/upgrade/vulnerable-passwords.in
+++ b/etc/upgrade/vulnerable-passwords.in
@@ -89,6 +89,9 @@ push @{$users->{'restrictions'}{ "main.Password" }}, "AND", {
value => '40',
};
+# we want to update passwords on disabled users
+$users->{'find_disabled_rows'} = 1;
+
my $count = $users->Count;
if ($count == 0) {
print "No users with unsalted or weak cryptography found.\n";
commit 928e123047291ffdad341cf4ea680e4f1ee32793
Author: Thomas Sibley <trs at bestpractical.com>
Date: Mon Mar 26 14:58:20 2012 -0400
Forbid javascript: and data: ticket links to avoid clickable XSS vectors
This partially resolves CVE-2011-2083. (#58605)
diff --git a/lib/RT/URI.pm b/lib/RT/URI.pm
index 4af1cb0..fce0459 100644
--- a/lib/RT/URI.pm
+++ b/lib/RT/URI.pm
@@ -130,7 +130,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 {
commit a2a50999aa214fa01bb824d2b6fcec197ec2a8e9
Author: Thomas Sibley <trs at bestpractical.com>
Date: Tue Mar 27 17:36:41 2012 -0400
Escape all arguments passed to /l
Escaping arguments prevents one of the easiest ways to accidentally
write XSS vectors. For /l calls which pass HTML in their arguments,
/l_unsafe is provided which preserves the previous behaviour. Note that
the text to be localized itself is not escaped, only the placeholder
values. Hopefully seeing "unsafe" in the name will also make folks
pause and consider what they're doing.
Partially resolves CVE-2011-2083.
diff --git a/share/html/Admin/Articles/Elements/Topics b/share/html/Admin/Articles/Elements/Topics
index 96ddaf0..43ca956 100644
--- a/share/html/Admin/Articles/Elements/Topics
+++ b/share/html/Admin/Articles/Elements/Topics
@@ -105,7 +105,7 @@ $topic
% }
% if ($Action) {
% unless ($Action eq "Move" and grep {$_->getNodeValue->Id == $Modify} $Element->getAllChildren) {
-<li><input type="submit" name="<%$Prefix%>-<%$topic eq "root" ? 0 : $topic->Id%>" value="<&|/l&><%$Action%> here</&>" /></li>
+<li><input type="submit" name="<%$Prefix%>-<%$topic eq "root" ? 0 : $topic->Id%>" value="<% $Action eq 'Move' ? loc('Move here') : loc('Add here') %>" /></li>
% }
% }
</ul>
diff --git a/share/html/Admin/CustomFields/Modify.html b/share/html/Admin/CustomFields/Modify.html
index eec4b1f..1dbc47f 100644
--- a/share/html/Admin/CustomFields/Modify.html
+++ b/share/html/Admin/CustomFields/Modify.html
@@ -105,7 +105,7 @@
<div class="hints">
<&|/l&>RT can make this custom field's values into hyperlinks to another service.</&>
<&|/l&>Fill in this field with a URL.</&>
-<&|/l, '<tt>__id__</tt>', '<tt>__CustomField__</tt>' &>RT will replace [_1] and [_2] with the record's id and the custom field's value, respectively.</&>
+<&|/l_unsafe, '<tt>__id__</tt>', '<tt>__CustomField__</tt>' &>RT will replace [_1] and [_2] with the record's id and the custom field's value, respectively.</&>
</div></td></tr>
<tr><td class="label"><&|/l&>Include page</&></td><td>
@@ -113,7 +113,7 @@
<div class="hints">
<&|/l&>RT can include content from another web service when showing this custom field.</&>
<&|/l&>Fill in this field with a URL.</&>
-<&|/l, '<tt>__id__</tt>', '<tt>__CustomField__</tt>' &>RT will replace [_1] and [_2] with the record's id and the custom field's value, respectively.</&>
+<&|/l_unsafe, '<tt>__id__</tt>', '<tt>__CustomField__</tt>' &>RT will replace [_1] and [_2] with the record's id and the custom field's value, respectively.</&>
<i><&|/l&>Some browsers may only load content from the same domain as your RT server.</&></i>
</div></td></tr>
diff --git a/share/html/Approvals/Elements/PendingMyApproval b/share/html/Approvals/Elements/PendingMyApproval
index 75ad5e1a..d2061da 100755
--- a/share/html/Approvals/Elements/PendingMyApproval
+++ b/share/html/Approvals/Elements/PendingMyApproval
@@ -63,9 +63,9 @@
<input type="checkbox" class="checkbox" value="1" name="ShowRejected" <% defined($ARGS{'ShowRejected'}) && $ARGS{'ShowRejected'} && qq[checked="checked"] |n%> /> <&|/l&>Show denied requests</&><br />
<input type="checkbox" class="checkbox" value="1" name="ShowDependent" <% defined($ARGS{'ShowDependent'}) && $ARGS{'ShowDependent'} && qq[checked="checked"] |n%> /> <&|/l&>Show requests awaiting other approvals</&><br />
-<&|/l, qq{<input size='15' class="ui-datepicker" value='}.($created_before->Unix > 0 &&$created_before->ISO(Timezone => 'user'))."' name='CreatedBefore' id='CreatedBefore' />"&>Only show approvals for requests created before [_1]</&><br />
+<&|/l_unsafe, qq{<input size='15' class="ui-datepicker" value='}.($created_before->Unix > 0 &&$created_before->ISO(Timezone => 'user'))."' name='CreatedBefore' id='CreatedBefore' />"&>Only show approvals for requests created before [_1]</&><br />
-<&|/l, qq{<input size='15' class="ui-datepicker" value='}.( $created_after->Unix >0 && $created_after->ISO(Timezone => 'user'))."' name='CreatedAfter' id='CreatedAfter' />"&>Only show approvals for requests created after [_1]</&>
+<&|/l_unsafe, qq{<input size='15' class="ui-datepicker" value='}.( $created_after->Unix >0 && $created_after->ISO(Timezone => 'user'))."' name='CreatedAfter' id='CreatedAfter' />"&>Only show approvals for requests created after [_1]</&>
</&>
<%init>
diff --git a/share/html/Elements/CreateTicket b/share/html/Elements/CreateTicket
index 6e541db..6702abc 100755
--- a/share/html/Elements/CreateTicket
+++ b/share/html/Elements/CreateTicket
@@ -51,7 +51,7 @@
% my $button_start = '<input type="submit" class="button" value="';
% my $button_end = '" />';
% my $queue_selector = $m->scomp('/Elements/SelectNewTicketQueue', OnChange => 'document.CreateTicketInQueue.submit()', SendTo => $SendTo );
-<&|/l, $button_start, $button_end, $queue_selector &>[_1]New ticket in[_2] [_3]</&>
+<&|/l_unsafe, $button_start, $button_end, $queue_selector &>[_1]New ticket in[_2] [_3]</&>
% $m->callback(CallbackName => 'BeforeFormEnd');
</form>
<%ARGS>
diff --git a/share/html/Elements/Footer b/share/html/Elements/Footer
index cbe8a0e..433a691 100755
--- a/share/html/Elements/Footer
+++ b/share/html/Elements/Footer
@@ -53,10 +53,10 @@
% if ($m->{'rt_base_time'}) {
<p id="time"><span><&|/l&>Time to display</&>: <%Time::HiRes::tv_interval( $m->{'rt_base_time'} )%></span></p>
%}
- <p id="bpscredits"><span><&|/l, '»|«', $RT::VERSION, '2012', '<a href="http://www.bestpractical.com?rt='.$RT::VERSION.'">Best Practical Solutions, LLC</a>', &>[_1] RT [_2] Copyright 1996-[_3] [_4].</&>
+ <p id="bpscredits"><span><&|/l_unsafe, '»|«', $RT::VERSION, '2012', '<a href="http://www.bestpractical.com?rt='.$RT::VERSION.'">Best Practical Solutions, LLC</a>', &>[_1] RT [_2] Copyright 1996-[_3] [_4].</&>
</span></p>
% if (!$Menu) {
- <p id="legal"><&|/l, '<a href="http://www.gnu.org/licenses/gpl-2.0.html">', '</a>' &>Distributed under [_1]version 2 of the GNU GPL[_2].</&><br /><&|/l, '<a href="mailto:sales at bestpractical.com">sales at bestpractical.com</a>' &>To inquire about support, training, custom development or licensing, please contact [_1].</&><br /></p>
+ <p id="legal"><&|/l_unsafe, '<a href="http://www.gnu.org/licenses/gpl-2.0.html">', '</a>' &>Distributed under [_1]version 2 of the GNU GPL[_2].</&><br /><&|/l_unsafe, '<a href="mailto:sales at bestpractical.com">sales at bestpractical.com</a>' &>To inquire about support, training, custom development or licensing, please contact [_1].</&><br /></p>
% }
</div>
% if ($Debug >= 2 ) {
diff --git a/share/html/Install/DatabaseType.html b/share/html/Install/DatabaseType.html
index 3312b57..68f8a67 100644
--- a/share/html/Install/DatabaseType.html
+++ b/share/html/Install/DatabaseType.html
@@ -58,7 +58,7 @@
<&|/l&>SQLite is a database that doesn't need a server or any configuration whatsoever. RT's authors recommend it for testing, demoing and development, but it's not quite right for a high-volume production RT server.</&>
</b></p>
<p>
-<&|/l, '<a href="http://search.cpan.org" target="_new">CPAN</a>' &>If your preferred database isn't listed in the dropdown below, that means RT couldn't find a <i>database driver</i> for it installed locally. You may be able to remedy this by using [_1] to download and install DBD::MySQL, DBD::Oracle or DBD::Pg.</&>
+<&|/l_unsafe, '<a href="http://search.cpan.org" target="_new">CPAN</a>' &>If your preferred database isn't listed in the dropdown below, that means RT couldn't find a <i>database driver</i> for it installed locally. You may be able to remedy this by using [_1] to download and install DBD::MySQL, DBD::Oracle or DBD::Pg.</&>
</p>
</div>
diff --git a/share/html/Install/Finish.html b/share/html/Install/Finish.html
index ee81e70..24ac0ff 100644
--- a/share/html/Install/Finish.html
+++ b/share/html/Install/Finish.html
@@ -53,7 +53,7 @@
</p>
<p>
-<&|/l, '<tt>root</tt>' &>You should be taken directly to a login page. You'll be able to log in with username of [_1] and the password you set earlier.</&>
+<&|/l_unsafe, '<tt>root</tt>' &>You should be taken directly to a login page. You'll be able to log in with username of [_1] and the password you set earlier.</&>
</p>
<p>
diff --git a/share/html/Search/Chart.html b/share/html/Search/Chart.html
index 884d183..070ce7c 100644
--- a/share/html/Search/Chart.html
+++ b/share/html/Search/Chart.html
@@ -124,7 +124,7 @@ my %query;
<input type="hidden" class="hidden" name="Query" value="<% $ARGS{Query} %>" />
<input type="hidden" class="hidden" name="SavedChartSearchId" value="<% $saved_search->{SearchId} || 'new' %>" />
-<&|/l, $m->scomp('Elements/SelectChartType', Name => 'ChartStyle', Default => $ChartStyle), $m->scomp('Elements/SelectGroupBy', Name => 'PrimaryGroupBy', Query => $ARGS{Query}, Default => $PrimaryGroupBy)
+<&|/l_unsafe, $m->scomp('Elements/SelectChartType', Name => 'ChartStyle', Default => $ChartStyle), $m->scomp('Elements/SelectGroupBy', Name => 'PrimaryGroupBy', Query => $ARGS{Query}, Default => $PrimaryGroupBy)
&>[_1] chart by [_2]</&><input type="submit" class="button" value="<%loc('Update Chart')%>" />
</form>
</&>
diff --git a/share/html/Search/Simple.html b/share/html/Search/Simple.html
index 07bd2f4..4d7b1e3 100644
--- a/share/html/Search/Simple.html
+++ b/share/html/Search/Simple.html
@@ -60,7 +60,7 @@
% my @strong = qw(<strong> </strong>);
-<p><&|/l, @strong &>Search for tickets by entering [_1]id[_2] numbers, subject words [_1]"in quotes"[_2], [_1]queues[_2] by name, Owners by [_1]username[_2], Requestors by [_1]email address[_2], and ticket [_1]statuses[_2].</&></p>
+<p><&|/l_unsafe, @strong &>Search for tickets by entering [_1]id[_2] numbers, subject words [_1]"in quotes"[_2], [_1]queues[_2] by name, Owners by [_1]username[_2], Requestors by [_1]email address[_2], and ticket [_1]statuses[_2].</&></p>
<p><&|/l&>Any word not recognized by RT is searched for in ticket subjects.</&></p>
@@ -74,7 +74,7 @@
% }
% }
-<p><&|/l, map { "<strong>$_</strong>" } qw(initial active inactive any) &>Entering [_1], [_2], [_3], or [_4] limits results to tickets with one of the respective types of statuses. Any individual status name limits results to just the statuses named.</&>
+<p><&|/l_unsafe, map { "<strong>$_</strong>" } qw(initial active inactive any) &>Entering [_1], [_2], [_3], or [_4] limits results to tickets with one of the respective types of statuses. Any individual status name limits results to just the statuses named.</&>
% if (RT->Config->Get('OnlySearchActiveTicketsInSimpleSearch', $session{'CurrentUser'})) {
% my $status_str = join ', ', map { loc($_) } RT::Queue->ActiveStatusArray;
@@ -82,13 +82,13 @@
% }
</p>
-<p><&|/l, map { "<strong>$_</strong>" } 'queue:"Example Queue"', 'owner:email at example.com' &>Start the search term with the name of a supported field followed by a colon, as in [_1] and [_2], to explicitly specify the search type.</&></p>
+<p><&|/l_unsafe, map { "<strong>$_</strong>" } 'queue:"Example Queue"', 'owner:email at example.com' &>Start the search term with the name of a supported field followed by a colon, as in [_1] and [_2], to explicitly specify the search type.</&></p>
-<p><&|/l, '<strong>cf.Name:value</strong>' &>CFs may be searched using a similar syntax as above with [_1].</&></p>
+<p><&|/l_unsafe, '<strong>cf.Name:value</strong>' &>CFs may be searched using a similar syntax as above with [_1].</&></p>
% my $link_start = '<a href="' . RT->Config->Get('WebPath') . '/Search/Build.html">';
% my $link_end = '</a>';
-<p><&|/l, $link_start, $link_end &>For the full power of RT's searches, please visit the [_1]search builder interface[_2].</&></p>
+<p><&|/l_unsafe, $link_start, $link_end &>For the full power of RT's searches, please visit the [_1]search builder interface[_2].</&></p>
</form>
diff --git a/share/html/l b/share/html/l
index 6396bc6..9f1b343 100755
--- a/share/html/l
+++ b/share/html/l
@@ -47,6 +47,6 @@
%# END BPS TAGGED BLOCK }}}
<%init>
my $hand = ($session{'CurrentUser'} ||= RT::CurrentUser->new)->LanguageHandle;
- $m->print($hand->maketext($m->content, at _));
+ $m->print($hand->maketext($m->content,map { $m->interp->apply_escapes($_, 'h') } @_));
return(1);
</%init>
diff --git a/share/html/l b/share/html/l_unsafe
similarity index 100%
copy from share/html/l
copy to share/html/l_unsafe
diff --git a/share/html/m/_elements/footer b/share/html/m/_elements/footer
index aad0020..0d160c2 100644
--- a/share/html/m/_elements/footer
+++ b/share/html/m/_elements/footer
@@ -48,7 +48,7 @@
<& /Elements/Logo, ShowName => 1, OnlyCustom => 1 &>
<div id="bpscredits">
<div id="copyright">
-<&|/l, '', '', '2012', '<a href="http://www.bestpractical.com?rt='.$RT::VERSION.'">Best Practical Solutions, LLC</a>', &>[_1] RT [_2] Copyright 1996-[_3] [_4].</&>
+<&|/l_unsafe, '', '', '2012', '<a href="http://www.bestpractical.com?rt='.$RT::VERSION.'">Best Practical Solutions, LLC</a>', &>[_1] RT [_2] Copyright 1996-[_3] [_4].</&>
</div>
</div>
</body>
commit 1792a7e43a5f01485f6e7ac337b1f425f50025f7
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Tue Mar 27 20:05:12 2012 -0400
Move menu initialization earlier in HandleRequest
This allows any of the Maybe... functions to display pages with menus,
and comes at essentially no cost.
diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index 01789ea..5f92257 100644
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -236,6 +236,7 @@ sub HandleRequest {
DecodeARGS($ARGS);
PreprocessTimeUpdates($ARGS);
+ InitializeMenu();
MaybeShowInstallModePage();
$HTML::Mason::Commands::m->comp( '/Elements/SetupSessionCookie', %$ARGS );
@@ -523,8 +524,6 @@ sub ShowRequestedPage {
# precache all system level rights for the current user
$HTML::Mason::Commands::session{CurrentUser}->PrincipalObj->HasRights( Object => RT->System );
- InitializeMenu();
-
SendSessionCookie();
# If the user isn't privileged, they can only see SelfService
commit 162cd0600533c6ebfd7cfe84c36f74ece6016f47
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Tue Mar 27 20:07:31 2012 -0400
Remove extra SendSessionCookie() calls
Calling SendSessionCookie() is only necessary after the cookie value has
changed, which only happens in two places -- initially, after the
current cookie is read, or after successful authentication, when we
change the session identifier to avoid session fixation. Additional
calls to SendSessionCookie only confuse when it is necessary.
diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index 5f92257..b86af34 100644
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -346,8 +346,6 @@ sub SetNextPage {
$HTML::Mason::Commands::session{'NextPage'}->{$hash} = $next;
$HTML::Mason::Commands::session{'i'}++;
-
- SendSessionCookie();
return $hash;
}
@@ -464,7 +462,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;
}
@@ -524,8 +521,6 @@ sub ShowRequestedPage {
# precache all system level rights for the current user
$HTML::Mason::Commands::session{CurrentUser}->PrincipalObj->HasRights( Object => RT->System );
- SendSessionCookie();
-
# If the user isn't privileged, they can only see SelfService
unless ( $HTML::Mason::Commands::session{'CurrentUser'}->Privileged ) {
commit c0b8291e9b9f6581dc57bb55c19938d61ec77bec
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Tue Mar 27 20:14:14 2012 -0400
Add basic HTTP_REFERER checking to prevent cross-site request forgery
RT has had a long-standing policy of accepting query parameters in both
POST and GET, and taking action based on them. Unfortunately, this
makes RT susceptible to cross-site request forgery (CSRF), wherein the
browser is tricked into making a request on behalf of the user, which
includes the user's cached credentials, and the remote server runs an
malicious action with the user's identity.
Prevent this by enforcing that all requests to RT come with a "Referer"
(sic) header of RT itself. Studies have shown that while the incidence
of suppressed or incorrect Referer headers is non-trivial over unsecured
HTTP (3% - 11%), it is very uncommon (0.05% - %0.22%) when the server is
secured using HTTPS[1]. As running RT without SSL protection already
presents a host of other vulnerabilities, including man-in-the-middle
attacks and password sniffing, we have judged that disabling CSRF
protections in HTTP-only deployments should be an acceptable solution.
Enforcing that all requests have such a header is entirely too
restrictive, as it prevents a large number of idempotent requests which
are not abusable, but provides the foundation of the CSRF protection.
This resolves CVE-2011-2085.
[1] http://seclab.stanford.edu/websec/csrf/csrf.pdf
diff --git a/etc/RT_Config.pm.in b/etc/RT_Config.pm.in
index 50b46c3..a3ea089 100755
--- a/etc/RT_Config.pm.in
+++ b/etc/RT_Config.pm.in
@@ -1758,8 +1758,21 @@ This disables RT's clickjacking protection.
Set($Framebusting, 1);
+=item C<$RestrictReferrer>
+
+If set to a false value, the HTTP C<Referer> (sic) header will not be
+checked to ensure that requests come from RT's own domain. As RT allows
+for GET requests to alter state, disabling this opens RT up to
+cross-site request forgery (CSRF) attacks.
+
+=cut
+
+Set($RestrictReferrer, 1);
+
=back
+
+
=head1 Authorization and user configuration
=over 4
diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index b86af34..1410d75 100644
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -286,6 +286,8 @@ sub HandleRequest {
}
}
+ MaybeDenyCSRF($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'} );
@@ -1120,6 +1122,41 @@ sub ComponentRoots {
return @roots;
}
+sub IsRefererCSRFWhitelisted {
+ my $referer = shift;
+
+ my $site = URI->new(RT->Config->Get('WebBaseURL'));
+ $site->host('127.0.0.1') if $site->host eq 'localhost';
+ return 1 if $referer->host_port eq $site->host_port;
+
+ return 0;
+}
+
+sub IsPossibleCSRF {
+ my $ARGS = shift;
+
+ # if there is no Referer header then assume the worst
+ return (1, "No Referer header. Perhaps your web browser is configured to never send the Referer header?") if !$ENV{HTTP_REFERER};
+
+ my $referer = URI->new($ENV{HTTP_REFERER});
+ $referer->host('127.0.0.1') if $referer->host eq 'localhost';
+
+ return 0 if IsRefererCSRFWhitelisted($referer);
+
+ return (1, "Referer is unknown site ".$referer->host_port);
+}
+
+sub MaybeDenyCSRF {
+ my $ARGS = shift;
+
+ return unless RT->Config->Get('RestrictReferrer');
+
+ my ($is_csrf, $msg) = IsPossibleCSRF($ARGS);
+ return if !$is_csrf;
+
+ HTML::Mason::Commands::Abort( $msg );
+}
+
package HTML::Mason::Commands;
use vars qw/$r $m %session/;
diff --git a/t/web/csrf.t b/t/web/csrf.t
new file mode 100644
index 0000000..45a722a
--- /dev/null
+++ b/t/web/csrf.t
@@ -0,0 +1,43 @@
+#!/usr/bin/perl
+use strict;
+use warnings;
+
+use RT::Test tests => undef;
+
+my ($baseurl, $m) = RT::Test->started_ok;
+
+my $test_page = "/Ticket/Create.html?Queue=1";
+
+$m->add_header(Referer => $baseurl);
+ok $m->login, 'logged in';
+
+# valid referer
+$m->add_header(Referer => $baseurl);
+$m->get_ok($test_page);
+$m->content_lacks("Referer is unknown site");
+$m->title_is('Create a new ticket');
+
+# now send a referer from an attacker
+$m->add_header(Referer => 'http://example.net');
+$m->get_ok($test_page);
+$m->content_contains("Referer is unknown site");
+$m->warning_like(qr/Referer is unknown site/);
+
+# now try self-service with CSRF
+my $user = RT::User->new(RT->SystemUser);
+$user->Create(Name => "SelfService", Password => "chops", Privileged => 0);
+
+$m = RT::Test::Web->new;
+$m->add_header(Referer => $baseurl);
+$m->get_ok("$baseurl/index.html?user=SelfService&pass=chops");
+$m->title_is("Open tickets", "got self-service interface");
+$m->content_contains("My open tickets", "got self-service interface");
+
+# post without referer
+$m->add_header(Referer => undef);
+$m->get_ok("/SelfService/Create.html?Queue=1");
+$m->content_contains("No Referer header");
+$m->warning_like(qr/No Referer header/);
+
+undef $m;
+done_testing;
commit 7b181889291137eeb74fa8e140bf1db895f820be
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Tue Mar 27 20:19:49 2012 -0400
Add a whitelist of idempotent request arguments
The previous commit introduced Referer enforcement; however, this
protection is entirely overzealous, as it prevents even the _first_
request to RT, and various other requests which would change no state on
the server.
Whitelist requests which contain no arguments, or which contain only
arguments which are assured to change no state on the server. This
allows initial visits to http://rt.example.com/ to pass the Referer
check, as well as allowing existing links in the form of
http://rt.example.com/Ticket/Display.html?id=1 to function.
diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index 1410d75..d3aadaf 100644
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -1122,6 +1122,29 @@ sub ComponentRoots {
return @roots;
}
+sub IsCompCSRFWhitelisted {
+ my $comp = shift;
+ my $ARGS = shift;
+
+ my %args = %{ $ARGS };
+
+ # 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}};
+
+ # 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 = shift;
@@ -1135,6 +1158,11 @@ sub IsRefererCSRFWhitelisted {
sub IsPossibleCSRF {
my $ARGS = shift;
+ return 0 if IsCompCSRFWhitelisted(
+ $HTML::Mason::Commands::m->request_comp->path,
+ $ARGS
+ );
+
# if there is no Referer header then assume the worst
return (1, "No Referer header. Perhaps your web browser is configured to never send the Referer header?") if !$ENV{HTTP_REFERER};
diff --git a/t/web/csrf.t b/t/web/csrf.t
index 45a722a..f55feb8 100644
--- a/t/web/csrf.t
+++ b/t/web/csrf.t
@@ -4,11 +4,15 @@ use warnings;
use RT::Test tests => undef;
+my $ticket = RT::Ticket->new(RT::CurrentUser->new('root'));
+my ($ok, $msg) = $ticket->Create(Queue => 1, Owner => 'nobody', Subject => 'bad music');
+ok($ok);
+
my ($baseurl, $m) = RT::Test->started_ok;
my $test_page = "/Ticket/Create.html?Queue=1";
-$m->add_header(Referer => $baseurl);
+$m->get_ok($baseurl);
ok $m->login, 'logged in';
# valid referer
@@ -23,12 +27,24 @@ $m->get_ok($test_page);
$m->content_contains("Referer is unknown site");
$m->warning_like(qr/Referer is unknown site/);
+# try a whitelisted argument from an attacker
+$m->add_header(Referer => 'http://example.net');
+$m->get_ok("/Ticket/Display.html?id=1");
+$m->content_lacks("Referer is unknown site");
+$m->title_is('#1: bad music');
+
+# now a non-whitelisted argument
+$m->get_ok("/Ticket/Display.html?id=1&Action=Take");
+$m->content_contains("Referer is unknown site");
+$m->warning_like(qr/Referer is unknown site/);
+
+
# now try self-service with CSRF
my $user = RT::User->new(RT->SystemUser);
$user->Create(Name => "SelfService", Password => "chops", Privileged => 0);
$m = RT::Test::Web->new;
-$m->add_header(Referer => $baseurl);
+$m->get_ok($baseurl);
$m->get_ok("$baseurl/index.html?user=SelfService&pass=chops");
$m->title_is("Open tickets", "got self-service interface");
$m->content_contains("My open tickets", "got self-service interface");
commit 3a7a6d9818aa0c5cee0f0718c45d9bdbb9ff729c
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Tue Mar 27 20:25:46 2012 -0400
Whitelist some component (not request!) paths
The RSS feed, despite being requested with no Referer header, and having
query parameters, modifies no state on the server, and as such should be
exempt from CSRF checks.
diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index d3aadaf..cadff51 100644
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -1122,10 +1122,19 @@ sub ComponentRoots {
return @roots;
}
+my %is_whitelisted_path = (
+ # 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_path{$comp};
+
my %args = %{ $ARGS };
# Eliminate arguments that do not indicate an effectful request.
commit c6669b25b173bcff6205f01231a9110e29b2179f
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Tue Mar 27 20:28:37 2012 -0400
Redirect to an interstitial page on CSRF attacks, rather than denying
As some browsers never supply Referer headers, and to maintain better
integration with other sites which may link into RT, simply denying the
a potential CSRF request with no recourse is unfortunate, and
frustrating to the user.
Instead, store the request parameters in a unique key on the session,
and provide a page with a link which will extract them again and re-run
the request. This allows intercepted requests to continue with only one
extra click.
diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index cadff51..20e6619 100644
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -286,7 +286,7 @@ sub HandleRequest {
}
}
- MaybeDenyCSRF($ARGS);
+ 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'}
@@ -1173,25 +1173,70 @@ sub IsPossibleCSRF {
);
# if there is no Referer header then assume the worst
- return (1, "No Referer header. Perhaps your web browser is configured to never send the Referer header?") if !$ENV{HTTP_REFERER};
+ return (1,
+ "your browser did not supply a Referrer header", # loc
+ ) if !$ENV{HTTP_REFERER};
my $referer = URI->new($ENV{HTTP_REFERER});
$referer->host('127.0.0.1') if $referer->host eq 'localhost';
-
return 0 if IsRefererCSRFWhitelisted($referer);
- return (1, "Referer is unknown site ".$referer->host_port);
+ return (1,
+ "the Referrer header supplied by your browser ([_1]) is not allowed", # loc
+ $referer->host_port);
}
-sub MaybeDenyCSRF {
+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->{path} eq $HTML::Mason::Commands::r->path_info;
+
+ my $user = $HTML::Mason::Commands::session{'CurrentUser'}->UserObj;
+ return unless $user->ValidateAuthString( $data->{auth}, $token );
+
+ %{$ARGS} = %{$data->{args}};
+
+ return 1;
+}
+
+sub MaybeShowInterstitialCSRFPage {
my $ARGS = shift;
return unless RT->Config->Get('RestrictReferrer');
- my ($is_csrf, $msg) = IsPossibleCSRF($ARGS);
+ # 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;
- HTML::Mason::Commands::Abort( $msg );
+ $RT::Logger->notice("Possible CSRF: ".RT::CurrentUser->new->loc($msg, @loc));
+
+ my $token = Digest::MD5::md5_hex(time . {} . $$ . rand(1024));
+ my $user = $HTML::Mason::Commands::session{'CurrentUser'}->UserObj;
+ my $data = {
+ auth => $user->GenerateAuthString( $token ),
+ path => $HTML::Mason::Commands::r->path_info,
+ args => $ARGS,
+ };
+
+ $HTML::Mason::Commands::session{'CSRF'}->{$token} = $data;
+ $HTML::Mason::Commands::session{'i'}++;
+
+ $HTML::Mason::Commands::m->comp(
+ '/Elements/CSRF',
+ OriginalURL => $HTML::Mason::Commands::r->path_info,
+ Reason => HTML::Mason::Commands::loc( $msg, @loc ),
+ Token => $token,
+ );
+ # Calls abort, never gets here
}
package HTML::Mason::Commands;
diff --git a/share/html/Elements/CSRF b/share/html/Elements/CSRF
new file mode 100644
index 0000000..ff658dd
--- /dev/null
+++ b/share/html/Elements/CSRF
@@ -0,0 +1,74 @@
+%# BEGIN BPS TAGGED BLOCK {{{
+%#
+%# COPYRIGHT:
+%#
+%# This software is Copyright (c) 1996-2012 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 }}}
+<& /Elements/Header, Title => loc('Possible cross-site request forgery') &>
+<& /Elements/Tabs &>
+
+<h1><&|/l&>Possible cross-site request forgery</&></h1>
+
+% my $strong_start = "<strong>";
+% my $strong_end = "</strong>";
+<p><&|/l, $strong_start, $strong_end, $Reason &>RT has detected a possible [_1]cross-site request forgery[_2] for this request, because [_3]. This is possibly caused by a malicious attacker trying to perform actions against RT on your behalf. If you did not initiate this request, then you should alert your security team.</&></p>
+
+% my $start = qq|<strong><a href="$url_with_token">|;
+% my $end = qq|</a></strong>|;
+<p><&|/l, $escaped_path, $start, $end &>If you really intended to visit [_1], then [_2]click here to resume your request[_3].</&></p>
+
+<& /Elements/Footer, %ARGS &>
+% $m->abort;
+<%ARGS>
+$OriginalURL => ''
+$Reason => ''
+$Token => ''
+</%ARGS>
+<%INIT>
+my $escaped_path = $m->interp->apply_escapes($OriginalURL, 'h');
+$escaped_path = "<tt>$escaped_path</tt>";
+
+my $url_with_token = URI->new($OriginalURL);
+$url_with_token->query_form([CSRF_Token => $Token]);
+</%INIT>
diff --git a/t/web/csrf.t b/t/web/csrf.t
index f55feb8..712289b 100644
--- a/t/web/csrf.t
+++ b/t/web/csrf.t
@@ -7,10 +7,13 @@ use RT::Test tests => undef;
my $ticket = RT::Ticket->new(RT::CurrentUser->new('root'));
my ($ok, $msg) = $ticket->Create(Queue => 1, Owner => 'nobody', Subject => 'bad music');
ok($ok);
+my $other = RT::Test->load_or_create_queue(Name => "Other queue", Disabled => 0);
+my $other_queue_id = $other->id;
my ($baseurl, $m) = RT::Test->started_ok;
my $test_page = "/Ticket/Create.html?Queue=1";
+my $test_path = "/Ticket/Create.html";
$m->get_ok($baseurl);
ok $m->login, 'logged in';
@@ -18,25 +21,102 @@ ok $m->login, 'logged in';
# valid referer
$m->add_header(Referer => $baseurl);
$m->get_ok($test_page);
-$m->content_lacks("Referer is unknown site");
+$m->content_lacks("Possible cross-site request forgery");
$m->title_is('Create a new ticket');
# now send a referer from an attacker
$m->add_header(Referer => 'http://example.net');
$m->get_ok($test_page);
-$m->content_contains("Referer is unknown site");
-$m->warning_like(qr/Referer is unknown site/);
+$m->content_contains("Possible cross-site request forgery");
+$m->content_contains("If you really intended to visit <tt>/Ticket/Create.html</tt>");
+$m->content_contains("the Referrer header supplied by your browser (example.net:80) is not allowed");
+$m->title_is('Possible cross-site request forgery');
+
+# reinstate mech's usual header policy
+$m->delete_header('Referer');
+
+# clicking the resume request button gets us to the test page
+$m->follow_link(text_regex => qr{resume your request});
+$m->content_lacks("Possible cross-site request forgery");
+like($m->response->request->uri, qr{^http://[^/]+\Q$test_path\E\?CSRF_Token=\w+$});
+$m->title_is('Create a new ticket');
# try a whitelisted argument from an attacker
$m->add_header(Referer => 'http://example.net');
$m->get_ok("/Ticket/Display.html?id=1");
-$m->content_lacks("Referer is unknown site");
+$m->content_lacks("Possible cross-site request forgery");
$m->title_is('#1: bad music');
# now a non-whitelisted argument
$m->get_ok("/Ticket/Display.html?id=1&Action=Take");
-$m->content_contains("Referer is unknown site");
-$m->warning_like(qr/Referer is unknown site/);
+$m->content_contains("Possible cross-site request forgery");
+$m->content_contains("If you really intended to visit <tt>/Ticket/Display.html</tt>");
+$m->content_contains("the Referrer header supplied by your browser (example.net:80) is not allowed");
+$m->title_is('Possible cross-site request forgery');
+
+$m->delete_header('Referer');
+$m->follow_link(text_regex => qr{resume your request});
+$m->content_lacks("Possible cross-site request forgery");
+like($m->response->request->uri, qr{^http://[^/]+\Q/Ticket/Display.html});
+$m->title_is('#1: bad music');
+$m->content_contains('Owner changed from Nobody to root');
+
+# force mech to never set referer
+$m->add_header(Referer => undef);
+$m->get_ok($test_page);
+$m->content_contains("Possible cross-site request forgery");
+$m->content_contains("If you really intended to visit <tt>/Ticket/Create.html</tt>");
+$m->content_contains("your browser did not supply a Referrer header");
+$m->title_is('Possible cross-site request forgery');
+
+$m->follow_link(text_regex => qr{resume your request});
+$m->content_lacks("Possible cross-site request forgery");
+is($m->response->redirects, 0, "no redirection");
+like($m->response->request->uri, qr{^http://[^/]+\Q$test_path\E\?CSRF_Token=\w+$});
+$m->title_is('Create a new ticket');
+
+# try sending the wrong csrf token, then the right one
+$m->add_header(Referer => undef);
+$m->get_ok($test_page);
+$m->content_contains("Possible cross-site request forgery");
+$m->content_contains("If you really intended to visit <tt>/Ticket/Create.html</tt>");
+$m->content_contains("your browser did not supply a Referrer header");
+$m->title_is('Possible cross-site request forgery');
+
+# Sending a wrong CSRF is just a normal request. We'll make a request
+# with just an invalid token, which means no Queue=, which means
+# Create.html errors out.
+my $link = $m->find_link(text_regex => qr{resume your request});
+(my $broken_url = $link->url) =~ s/(CSRF_Token)=\w+/$1=crud/;
+$m->get_ok($broken_url);
+$m->content_contains("Queue could not be loaded");
+$m->title_is('RT Error');
+$m->warning_like(qr/Queue could not be loaded/);
+
+# The token doesn't work for other pages, or other arguments to the same page.
+$m->add_header(Referer => undef);
+$m->get_ok($test_page);
+$m->content_contains("Possible cross-site request forgery");
+my ($token) = $m->content =~ m{CSRF_Token=(\w+)};
+
+$m->add_header(Referer => undef);
+$m->get_ok("/Admin/Queues/Modify.html?id=new&Name=test&CSRF_Token=$token");
+$m->content_contains("Possible cross-site request forgery");
+$m->content_contains("If you really intended to visit <tt>/Admin/Queues/Modify.html</tt>");
+$m->content_contains("your browser did not supply a Referrer header");
+$m->title_is('Possible cross-site request forgery');
+
+$m->follow_link(text_regex => qr{resume your request});
+$m->content_lacks("Possible cross-site request forgery");
+$m->title_is('Configuration for queue test');
+
+# Try the same page, but different query parameters, which are blatted by the token
+$m->get_ok("/Ticket/Create.html?Queue=$other_queue_id&CSRF_Token=$token");
+$m->content_lacks("Possible cross-site request forgery");
+$m->title_is('Create a new ticket');
+$m->text_unlike(qr/Queue:\s*Other queue/);
+$m->text_like(qr/Queue:\s*General/);
+
# now try self-service with CSRF
@@ -52,8 +132,17 @@ $m->content_contains("My open tickets", "got self-service interface");
# post without referer
$m->add_header(Referer => undef);
$m->get_ok("/SelfService/Create.html?Queue=1");
-$m->content_contains("No Referer header");
-$m->warning_like(qr/No Referer header/);
+$m->content_contains("Possible cross-site request forgery");
+$m->content_contains("If you really intended to visit <tt>/SelfService/Create.html</tt>");
+$m->content_contains("your browser did not supply a Referrer header");
+$m->title_is('Possible cross-site request forgery');
+
+$m->follow_link(text_regex => qr{resume your request});
+$m->content_lacks("Possible cross-site request forgery");
+is($m->response->redirects, 0, "no redirection");
+like($m->response->request->uri, qr{^http://[^/]+\Q/SelfService/Create.html\E\?CSRF_Token=\w+$});
+$m->title_is('Create a ticket');
+$m->content_contains('Describe the issue below:');
undef $m;
done_testing;
commit cb662a572320ea7df39adb87c8e6e4243bdfa95c
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Tue Mar 27 20:29:37 2012 -0400
Allow file uploads to persist across CSRF interstitial
While storing request parameters into the session is sufficient for most
requests, it does not cover the case of file uploads, when the request
arguments contain a filehandle, and thus cannot be cleanly serialized.
Deal with this, in the case of ticket attachments, by parsing and
storing the attachment in the CSRF session data using MakeMIMEEntity, as
the request would usually have done. On the event of the CSRF token
being used, extract the parsed attachment and insert it into the
canonical session storage for attachments.
This does not cover the case of custom field file uploads, as the latter
are never stored in the session, and are much harder to isolate.
diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index 20e6619..b50802c 100644
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -1201,6 +1201,16 @@ sub ExpandCSRFToken {
%{$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;
}
@@ -1226,6 +1236,14 @@ sub MaybeShowInterstitialCSRFPage {
path => $HTML::Mason::Commands::r->path_info,
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'}++;
diff --git a/t/web/csrf.t b/t/web/csrf.t
index 712289b..ceb0c3d 100644
--- a/t/web/csrf.t
+++ b/t/web/csrf.t
@@ -117,6 +117,29 @@ $m->title_is('Create a new ticket');
$m->text_unlike(qr/Queue:\s*Other queue/);
$m->text_like(qr/Queue:\s*General/);
+# Ensure that file uploads work across the interstitial
+$m->delete_header('Referer');
+$m->get_ok($test_page);
+$m->content_contains("Create a new ticket", 'ticket create page');
+$m->form_name('TicketCreate');
+$m->field('Subject', 'Attachments test');
+
+my $logofile = "$RT::MasonComponentRoot/NoAuth/images/bpslogo.png";
+open LOGO, "<", $logofile or die "Can't open logo file: $!";
+binmode LOGO;
+my $logo_contents = do {local $/; <LOGO>};
+close LOGO;
+$m->field('Attach', $logofile);
+
+# Lose the referer before the POST
+$m->add_header(Referer => undef);
+$m->submit;
+$m->content_contains("Possible cross-site request forgery");
+$m->content_contains("If you really intended to visit <tt>/Ticket/Create.html</tt>");
+$m->follow_link(text_regex => qr{resume your request});
+$m->content_contains('Download bpslogo.png', 'page has file name');
+$m->follow_link_ok({text => "Download bpslogo.png"});
+is($m->content, $logo_contents, "Binary content matches");
# now try self-service with CSRF
commit 7d661a575463722a4d8ec7972c504b1f1829bb68
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Tue Mar 27 20:31:19 2012 -0400
Add optional CSRF login protection
RT has historically allowed logins from any location by simply providing
'user' and 'pass' parameters, which were used to log the user in. This
opens RT to "login CSRF," wherein a user is tricked into logging in
using the _attacker_'s credentials, which allows the attacker to later
examine their own account and possibly extract information about the
user's browsing history.
As RT does not track or display such information about the user, and the
ability to provide URLs which log a user into RT is central to many
workflows, explicitly whitelist 'user' and 'pass' parameters to RT for
CSRF purposes, if (and only if) they authenticate correctly. If so, it
is also sufficient to whitelist the entire request, as it proves that
the requesting entity possesses the user's full credentials, and would
be able to take any action regardless.
This exception will not function if RT is configured to authenticate
against a source other than its internal database, such as any of those
provided by RT::Authen::ExternalAuth, or via the $ExternalAuth
configuration setting, as RT has no way of verifying the credentials
provided via the query parameters.
If login CSRF is deemed a potential issue locally, we provide a
configuration option ($RestrictLoginReferrer) to enable login CSRF
protection.
diff --git a/etc/RT_Config.pm.in b/etc/RT_Config.pm.in
index a3ea089..c560c5e 100755
--- a/etc/RT_Config.pm.in
+++ b/etc/RT_Config.pm.in
@@ -1769,6 +1769,18 @@ cross-site request forgery (CSRF) attacks.
Set($RestrictReferrer, 1);
+=item C<$RestrictLoginReferrer>
+
+If set to a false value, RT will allow the user to log in from any link
+or request, merely by passing in C<user> and C<pass> parameters; setting
+it to a true value forces all logins to come from the login box, so the
+user us aware that they are being logged in. The default is off, for
+backwards compatability.
+
+=cut
+
+Set($RestrictLoginReferrer, 0);
+
=back
diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index b50802c..2341d60 100644
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -1137,6 +1137,20 @@ sub IsCompCSRFWhitelisted {
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.
diff --git a/t/web/csrf.t b/t/web/csrf.t
index ceb0c3d..d99b4ce 100644
--- a/t/web/csrf.t
+++ b/t/web/csrf.t
@@ -15,7 +15,6 @@ my ($baseurl, $m) = RT::Test->started_ok;
my $test_page = "/Ticket/Create.html?Queue=1";
my $test_path = "/Ticket/Create.html";
-$m->get_ok($baseurl);
ok $m->login, 'logged in';
# valid referer
@@ -24,6 +23,18 @@ $m->get_ok($test_page);
$m->content_lacks("Possible cross-site request forgery");
$m->title_is('Create a new ticket');
+# off-site referer BUT provides auth
+$m->add_header(Referer => 'http://example.net');
+$m->get_ok("$test_page&user=root&pass=password");
+$m->content_lacks("Possible cross-site request forgery");
+$m->title_is('Create a new ticket');
+
+# explicitly no referer BUT provides auth
+$m->add_header(Referer => undef);
+$m->get_ok("$test_page&user=root&pass=password");
+$m->content_lacks("Possible cross-site request forgery");
+$m->title_is('Create a new ticket');
+
# now send a referer from an attacker
$m->add_header(Referer => 'http://example.net');
$m->get_ok($test_page);
@@ -147,7 +158,6 @@ my $user = RT::User->new(RT->SystemUser);
$user->Create(Name => "SelfService", Password => "chops", Privileged => 0);
$m = RT::Test::Web->new;
-$m->get_ok($baseurl);
$m->get_ok("$baseurl/index.html?user=SelfService&pass=chops");
$m->title_is("Open tickets", "got self-service interface");
$m->content_contains("My open tickets", "got self-service interface");
commit 6ef92feaade1d8009bea08f0cb9f1ce8134714e5
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Tue Mar 27 20:38:01 2012 -0400
Allow REST requests to function regardless of Referer header
The REST interface, and existing clients to it, expect to be able to
make queries without supplying a Referer parameter -- which CSRF
protections should rightly deny. While the simple solution would be to
include a Referer header in such requests, we cannot assume that all
REST clients can be so updated.
Create a secondary class of session, the REST session, which is allowed
to make Referer-less state-modifying requests, but not access the rest
of the UI. We recognize such sessions by if their initial request is to
a REST URL; if so, we mark the session as a REST session. For clients
with existing sessions which are not flagged as REST or not, we assign
them into the appropriate class based on the first request we observe;
this will allow existing clients with long-running sessions to continue
unmodified.
This also removes a SessionType check which became obsolete in 54f0f73,
and re-implements the functionality using the new REST flag on the
session.
diff --git a/bin/rt-mailgate.in b/bin/rt-mailgate.in
index 9ad129a..6e5f4a1 100755
--- a/bin/rt-mailgate.in
+++ b/bin/rt-mailgate.in
@@ -172,7 +172,6 @@ sub setup_session {
my $self = shift;
my $opts = shift;
my %post_params;
- $post_params{SessionType} = 'REST'; # Surpress login box
foreach (qw(queue action)) {
$post_params{$_} = $opts->{$_} if defined $opts->{$_};
}
diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index 2341d60..ecc1afe 100644
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -1181,6 +1181,29 @@ sub IsRefererCSRFWhitelisted {
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 $path = $HTML::Mason::Commands::r->path_info;
+ $HTML::Mason::Commands::session{'REST'} = $path =~ m{^/+REST/\d+\.\d+(/|$)}
+ unless defined $HTML::Mason::Commands::session{'REST'};
+
+ if ($HTML::Mason::Commands::session{'REST'}) {
+ return 0 if $path =~ 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(
$HTML::Mason::Commands::m->request_comp->path,
$ARGS
diff --git a/share/html/Elements/Error b/share/html/Elements/Error
index 50f3b77..87dfd02 100755
--- a/share/html/Elements/Error
+++ b/share/html/Elements/Error
@@ -81,7 +81,7 @@ Encode::_utf8_off($error);
$RT::Logger->error($error);
-if ( defined $session{'SessionType'} && $session{'SessionType'} eq 'REST' ) {
+if ( $session{'REST'} ) {
$r->content_type('text/plain');
$m->out( "Error: " . $Why . "\n" );
$m->out( $Details . "\n" ) if defined $Details && length $Details;
diff --git a/t/web/csrf-rest.t b/t/web/csrf-rest.t
new file mode 100644
index 0000000..5bb9081
--- /dev/null
+++ b/t/web/csrf-rest.t
@@ -0,0 +1,77 @@
+#!/usr/bin/perl
+use strict;
+use warnings;
+
+use RT::Test tests => undef;
+
+my ($baseurl, $m) = RT::Test->started_ok;
+
+# Get a non-REST session
+diag "Standard web session";
+ok $m->login, 'logged in';
+$m->content_contains("RT at a glance", "Get full UI content");
+
+# Requesting a REST page should be fine, as we have a Referer
+$m->post("$baseurl/REST/1.0/ticket/new", [
+ format => 'l',
+]);
+$m->content_like(qr{^id: ticket/new}m, "REST request with referrer");
+
+# Removing the Referer header gets us an interstitial
+$m->add_header(Referer => undef);
+$m->post("$baseurl/REST/1.0/ticket/new", [
+ format => 'l',
+ foo => 'bar',
+]);
+$m->content_contains("Possible cross-site request forgery",
+ "REST request without referrer is blocked");
+
+# But passing username and password lets us though
+$m->post("$baseurl/REST/1.0/ticket/new", [
+ user => 'root',
+ pass => 'password',
+ format => 'l',
+]);
+$m->content_like(qr{^id: ticket/new}m, "REST request without referrer, but username/password supplied, is OK");
+
+# And we can still access non-REST urls
+$m->get("$baseurl");
+$m->content_contains("RT at a glance", "Full UI is still available");
+
+
+# Now go get a REST session
+diag "REST session";
+$m = RT::Test::Web->new;
+$m->post("$baseurl/REST/1.0/ticket/new", [
+ user => 'root',
+ pass => 'password',
+ format => 'l',
+]);
+$m->content_like(qr{^id: ticket/new}m, "REST request to log in");
+
+# Requesting that page again, with a username/password but no referrer,
+# is fine
+$m->add_header(Referer => undef);
+$m->post("$baseurl/REST/1.0/ticket/new", [
+ user => 'root',
+ pass => 'password',
+ format => 'l',
+]);
+$m->content_like(qr{^id: ticket/new}m, "REST request with no referrer, but username/pass");
+
+# And it's still fine without both referer and username and password,
+# because REST is special-cased
+$m->post("$baseurl/REST/1.0/ticket/new", [
+ format => 'l',
+]);
+$m->content_like(qr{^id: ticket/new}m, "REST request with no referrer or username/pass is special-cased for REST sessions");
+
+# But the REST page can't request normal pages
+$m->get("$baseurl");
+$m->content_lacks("RT at a glance", "Full UI is denied for REST sessions");
+$m->content_contains("This login session belongs to a REST client", "Tells you why");
+$m->warning_like(qr/This login session belongs to a REST client/, "Logs a warning");
+
+undef $m;
+done_testing;
+
diff --git a/t/web/redirect-after-login.t b/t/web/redirect-after-login.t
index d429d30..835b24c 100644
--- a/t/web/redirect-after-login.t
+++ b/t/web/redirect-after-login.t
@@ -196,16 +196,17 @@ for my $path (qw(Prefs/Other.html /Prefs/Other.html)) {
# test REST login response
{
+ $agent = RT::Test::Web->new;
my $requested = $url."REST/1.0/?user=root;pass=password";
$agent->get($requested);
is($agent->status, 200, "Loaded a page");
is($agent->uri, $requested, "didn't redirect to /NoAuth/Login.html for REST");
- $agent->get_ok($url);
- $agent->logout();
+ $agent->get_ok($url."REST/1.0");
}
# test REST login response for wrong pass
{
+ $agent = RT::Test::Web->new;
my $requested = $url."REST/1.0/?user=root;pass=passwrong";
$agent->get_ok($requested);
is($agent->status, 200, "Loaded a page");
@@ -217,6 +218,7 @@ for my $path (qw(Prefs/Other.html /Prefs/Other.html)) {
# test REST login response for no creds
{
+ $agent = RT::Test::Web->new;
my $requested = $url."REST/1.0/";
$agent->get_ok($requested);
is($agent->status, 200, "Loaded a page");
commit 2ad1bcc658c38c7be44de7aff54b7199975ab5b6
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Mon Nov 14 17:22:35 2011 -0500
Prevent storing the old or new hashed password in the transaction table
diff --git a/lib/RT/User.pm b/lib/RT/User.pm
index 8a82377..29eae70 100644
--- a/lib/RT/User.pm
+++ b/lib/RT/User.pm
@@ -1447,7 +1447,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'},
commit 1d838609a9dfa35dc9e05b088a79cf7a5f8e8a3d
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Mon Nov 14 17:29:39 2011 -0500
Clean out sensitive user transactions
diff --git a/etc/upgrade/4.0.6/content b/etc/upgrade/4.0.6/content
new file mode 100644
index 0000000..dc1a009
--- /dev/null
+++ b/etc/upgrade/4.0.6/content
@@ -0,0 +1,17 @@
+ at Initial = (
+ sub {
+ my $txns = RT::Transactions->new( $RT::SystemUser );
+ $txns->Limit(
+ FIELD => "ObjectType",
+ VALUE => "RT::User",
+ );
+ $txns->Limit(
+ FIELD => "Field",
+ VALUE => "Password",
+ );
+ while (my $txn = $txns->Next) {
+ $txn->__Set( Field => $_, Value => '********' )
+ for qw/OldValue NewValue/;
+ }
+ },
+);
commit 057463bc9914d8d6472a2a08009caefb2f8cdc53
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Fri Nov 18 16:57:56 2011 -0500
Add a consistent CurrentUserCanSee right
diff --git a/lib/RT/Article.pm b/lib/RT/Article.pm
index 7310241..24b952a 100644
--- a/lib/RT/Article.pm
+++ b/lib/RT/Article.pm
@@ -543,6 +543,17 @@ sub CurrentUserHasRight {
}
+=head2 CurrentUserCanSee
+
+Returns true if the current user can see the article, using ShowArticle
+
+=cut
+
+sub CurrentUserCanSee {
+ my $self = shift;
+ return $self->CurrentUserHasRight('ShowArticle');
+}
+
# }}}
# {{{ _Set
diff --git a/lib/RT/Group.pm b/lib/RT/Group.pm
index 779c026..b367b2f 100644
--- a/lib/RT/Group.pm
+++ b/lib/RT/Group.pm
@@ -1171,8 +1171,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;
+}
=head2 PrincipalObj
diff --git a/lib/RT/Queue.pm b/lib/RT/Queue.pm
index 3cb87c4..ab9d460 100644
--- a/lib/RT/Queue.pm
+++ b/lib/RT/Queue.pm
@@ -1249,6 +1249,17 @@ 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');
+}
=head2 HasRight
diff --git a/lib/RT/Ticket.pm b/lib/RT/Ticket.pm
index 3f2e94c..f960eea 100644
--- a/lib/RT/Ticket.pm
+++ b/lib/RT/Ticket.pm
@@ -3513,6 +3513,16 @@ 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');
+}
=head2 HasRight
diff --git a/lib/RT/User.pm b/lib/RT/User.pm
index 29eae70..0cb4a18 100644
--- a/lib/RT/User.pm
+++ b/lib/RT/User.pm
@@ -1206,6 +1206,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
@@ -1475,25 +1506,9 @@ sub _Value {
my $self = shift;
my $field = shift;
- #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
commit 5a927993be1a33d1837bc7ab21836fb29206d278
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Fri Nov 18 16:59:52 2011 -0500
Enable ACL checks for non-Ticket transactions
Use CurrentUserCanSee on relevant object to prevent access to
Transactions that the user shouldn't be able to see.
This partially resolves CVE-2011-2084.
diff --git a/lib/RT/Transaction.pm b/lib/RT/Transaction.pm
index 21ccaee..a9673f3 100644
--- a/lib/RT/Transaction.pm
+++ b/lib/RT/Transaction.pm
@@ -1066,14 +1066,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");
}
commit 619d19d8f5ff9200220742db5d0352b77c9755ea
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Fri Nov 18 20:47:21 2011 -0500
Remove unused $args and @arglist variables
diff --git a/share/html/REST/1.0/Forms/transaction/default b/share/html/REST/1.0/Forms/transaction/default
index 1ffa2b2..2e45f67 100644
--- a/share/html/REST/1.0/Forms/transaction/default
+++ b/share/html/REST/1.0/Forms/transaction/default
@@ -49,7 +49,6 @@
%#
<%ARGS>
$id
-$args => undef
$format => undef
$fields => undef
</%ARGS>
@@ -57,8 +56,6 @@ $fields => undef
my $trans = RT::Transactions->new($session{CurrentUser});
my ($c, $o, $k, $e) = ("", [], {} , "");
-chomp $args;
-my @arglist = split('/', $args);
my $tid = $id;
$trans->Limit(FIELD => 'Id', OPERATOR => '=', VALUE => $tid);
commit f8eafa6e6bf951ffade5abf62682204b7acd2e77
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Fri Nov 18 20:48:07 2011 -0500
Explicitly ACL ObjectCustomFieldValue content, based on the custon field object
diff --git a/lib/RT/ObjectCustomFieldValue.pm b/lib/RT/ObjectCustomFieldValue.pm
index 0fd9d73..0bff551 100644
--- a/lib/RT/ObjectCustomFieldValue.pm
+++ b/lib/RT/ObjectCustomFieldValue.pm
@@ -251,6 +251,8 @@ my $re_ip_serialized = qr/$re_ip_sunit(?:\.$re_ip_sunit){3}/;
sub Content {
my $self = shift;
+ return undef unless $self->CustomFieldObj->CurrentUserHasRight('SeeCustomField');
+
my $content = $self->_Value('Content');
if ( $self->CustomFieldObj->Type eq 'IPAddress'
|| $self->CustomFieldObj->Type eq 'IPAddressRange' )
commit 59d2fb3ad38acf6614515aef0e7e2e5ba7c5634d
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Fri Nov 18 20:49:25 2011 -0500
There is no reason for ->NewValue and ->OldValue to skip ACLs via __Value
diff --git a/lib/RT/Transaction.pm b/lib/RT/Transaction.pm
index a9673f3..a7d14a1 100644
--- a/lib/RT/Transaction.pm
+++ b/lib/RT/Transaction.pm
@@ -1091,7 +1091,7 @@ sub OldValue {
return $Object->Content;
}
else {
- return $self->__Value('OldValue');
+ return $self->_Value('OldValue');
}
}
@@ -1105,7 +1105,7 @@ sub NewValue {
return $Object->Content;
}
else {
- return $self->__Value('NewValue');
+ return $self->_Value('NewValue');
}
}
commit 58ac3d2ebe46394d10ebdf413f287aea73f2a646
Author: Thomas Sibley <trs at bestpractical.com>
Date: Mon Mar 26 17:42:23 2012 -0400
Check ACLs on the receiving end when modifying a scrip's Queue or Template
Users with ModifyScrips in Queue A must also have ModifyScrips in the
receiving queue when moving a scrip from one queue to another. When
making a scrip global, the actor must have ModifyScrips globally.
Similarly, users must be able to see the template they're updating the
scrip to use.
This stricter ACL checking prevents queue admins from moving arbitrary
scrips into other queues in which they have no permissions.
Partially resolves CVE-2011-2084. Ticket #50901.
diff --git a/lib/RT/Scrip.pm b/lib/RT/Scrip.pm
index 3e8f352..0e0c7a0 100644
--- a/lib/RT/Scrip.pm
+++ b/lib/RT/Scrip.pm
@@ -510,13 +510,35 @@ sub _Set {
}
- if (length($args{Value})) {
+ if (exists $args{Value}) {
if ($args{Field} eq 'CustomIsApplicableCode' || $args{Field} eq 'CustomPrepareCode' || $args{Field} eq 'CustomCommitCode') {
unless ( $self->CurrentUser->HasRight( Object => $RT::System,
Right => 'ExecuteCode' ) ) {
return ( 0, $self->loc('Permission Denied') );
}
}
+ elsif ($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->CurrentUserCanRead) {
+ return ( 0, $self->loc('Permission Denied') );
+ }
+ }
}
return $self->__Set(@_);
commit 08b7989feee46bbd95d253714fb90e112d37aa3a
Author: Thomas Sibley <trs at bestpractical.com>
Date: Tue Mar 27 12:28:41 2012 -0400
Check ACLs on the receiving end when modifying a Template's Queue
Users with ModifyTemplate in Queue A must also have ModifyTemplate in the
receiving queue when moving a template from one queue to another. When
making a template global, the actor must have ModifyTemplate globally.
This stricter ACL checking prevents queue admins from moving arbitrary
templates into other queues in which they have no permissions.
Partially resolves CVE-2011-2084. Ticket #50901.
diff --git a/lib/RT/Template.pm b/lib/RT/Template.pm
index 158547a..117cc3f 100644
--- a/lib/RT/Template.pm
+++ b/lib/RT/Template.pm
@@ -96,10 +96,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( @_ );
}
commit 6221350f2ca27615ed5ef6b87b1d3ff76f16463f
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Tue Nov 15 00:43:25 2011 -0500
Ignore the local directory which contains additional, temporarily non-public tests
diff --git a/.gitignore b/.gitignore
index 4fbbaa6..f62dd86 100644
--- a/.gitignore
+++ b/.gitignore
@@ -13,6 +13,7 @@
/t/data/gnupg/keyrings/random_seed
/t/data/configs/apache2.2+fastcgi.conf
/t/data/configs/apache2.2+mod_perl.conf
+/t/security/embargo/
/t/tmp/
/sbin/rt-attributes-viewer
/sbin/rt-clean-sessions
commit 3718c5ea1b1e988980a03a8bbdf93a214add5152
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Fri Mar 30 18:36:02 2012 -0400
Ensure that the new /l_unsafe is protected from direct access as well
diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index 01789ea..80e4075 100644
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -490,7 +490,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) {
$m->abort(403);
commit 4209699a3f6301c3e95e70216cb80c848f8133e0
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Tue Nov 15 14:50:28 2011 -0500
Add a note about the timeline on public announcements, tests, etc
diff --git a/docs/security.pod b/docs/security.pod
index b8650e0..620f868 100644
--- a/docs/security.pod
+++ b/docs/security.pod
@@ -9,6 +9,21 @@ key).
More information is available at L<http://bestpractical.com/security/>.
+
+=head2 RT's security process
+
+After a security vulnerability is reported to Best Practical and
+verified, we attempt to resolve it in as timely a fashion as possible.
+Best Practical support customers will be notified before we disclose the
+information to the public. All security announcements will be sent to
+C<rt-announce at bestpractical.com>, which includes
+C<rt-users at bestpractical.com> and C<rt-devel at bestpractical.com>.
+
+As the tests for security vulnerabilities are often nearly identical to
+working exploits, sensitive tests will be embargoed for a period of six
+months before being added to the public RT repository.
+
+
=head2 Security tips for running RT
=over
commit 4881ae828fa604dc2b7df6531c93654b104f8909
Author: Thomas Sibley <trs at bestpractical.com>
Date: Thu Mar 29 14:12:37 2012 -0400
RowsPerPage and FirstRow only accept natural numbers and undef
This prevents user provided page sizes from injecting SQL after the
LIMIT. GotoPage is covered by FirstRow.
Resolves CVE-2011-4460. Ticket #69239.
diff --git a/lib/RT/SearchBuilder.pm b/lib/RT/SearchBuilder.pm
index aa2e25a..5ee7ecb 100644
--- a/lib/RT/SearchBuilder.pm
+++ b/lib/RT/SearchBuilder.pm
@@ -131,6 +131,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
commit 74ab1eaab2ca78c7d8b3a451167c88bcb4ec1335
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Thu Nov 17 20:14:07 2011 -0500
Avoid shell interpolation when calling sendmailpipe
Use IPC::Open2 rather than three-arg open with "|-", to prevent shell
interpolation. This allows not only fine control of the parameters to
the sendmail command, but also allows the STDOUT content to be dropped,
as was previously handled using a shell output redirect. For backwards
compatability, use Text::ParseWords to split the configuration
parameters which are strings, rather than argument lists.
This prevents a vulnerability where specially-crafted emails could allow
code execution if VERP was enabled.
This resolves CVE-2011-2082.
diff --git a/lib/RT/Interface/Email.pm b/lib/RT/Interface/Email.pm
index 909a9f4..385ba72 100644
--- a/lib/RT/Interface/Email.pm
+++ b/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,11 +405,11 @@ 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 and bounces conflict, since they both want -f
if ( $args{'Bounce'} ) {
- $args .= ' '. RT->Config->Get('SendmailBounceArguments');
+ push @args, shellwords(RT->Config->Get('SendmailBounceArguments'));
} elsif ( RT->Config->Get('SetOutgoingMailFrom') ) {
my $OutgoingMailAddress;
@@ -425,7 +426,7 @@ sub SendEmail {
$OutgoingMailAddress ||= RT->Config->Get('OverrideOutgoingMailFrom')->{'Default'};
- $args .= " -f $OutgoingMailAddress"
+ push @args, "-f", $OutgoingMailAddress
if $OutgoingMailAddress;
}
@@ -437,32 +438,36 @@ 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" };
+
+ 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 );
}
commit 87aa1d4fd8f07aaeb54cb54f23f40c935e23e897
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Mon Apr 4 16:12:47 2011 -0400
Always pass in status list to selfservice search
This prevents it from being overridden via query parameters; much like
the previous commit, this allowed TicketSQL injection. At the same
time, properly escape any statuses which contain apostrophes.
diff --git a/share/html/SelfService/Elements/MyRequests b/share/html/SelfService/Elements/MyRequests
index a7b453d..880b4e3 100755
--- a/share/html/SelfService/Elements/MyRequests
+++ b/share/html/SelfService/Elements/MyRequests
@@ -63,15 +63,14 @@ my $title = loc("My [_1] tickets", $friendly_status);
my $id = $session{'CurrentUser'}->id;
my $Query = "( Watcher.id = $id )";
if ( @status ) {
- $Query .= " AND ( "
- . join( ' OR ', map "Status = '$_'", @status )
- . " )";
+ @status = map {s/(['\\])/\\$1/g; "Status = '$_'"} @status;
+ $Query .= " AND ( " . join(' OR ', @status ) . " )";
}
my $Format = RT->Config->Get('DefaultSelfServiceSearchResultFormat');
</%INIT>
<%ARGS>
$friendly_status => loc('open')
- at status => RT::Queue->ActiveStatusArray()
+ at status => ()
$BaseURL => undef
$Page => 1
@Order => ('ASC')
diff --git a/share/html/SelfService/index.html b/share/html/SelfService/index.html
index f57554a..9030804 100755
--- a/share/html/SelfService/index.html
+++ b/share/html/SelfService/index.html
@@ -48,6 +48,8 @@
<& /SelfService/Elements/Header, Title => loc('Open tickets') &>
<& /SelfService/Elements/MyRequests,
%ARGS,
+ status => [ RT::Queue->ActiveStatusArray() ],
+ friendly_status => loc('open'),
BaseURL => RT->Config->Get('WebPath') ."/SelfService/?",
Page => $Page,
&>
commit 3ee90284f10067c9d1a29b7a1d09338e308a76be
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Mon Apr 2 16:17:40 2012 -0400
Update test to catch the new warning
diff --git a/t/api/date.t b/t/api/date.t
index 9756e51..6fcaa49 100644
--- a/t/api/date.t
+++ b/t/api/date.t
@@ -4,7 +4,7 @@ use Test::MockTime qw(set_fixed_time restore_time);
use DateTime;
use warnings; use strict;
-use RT::Test tests => 172;
+use RT::Test tests => 173;
use RT::User;
use Test::Warn;
@@ -85,9 +85,11 @@ my $current_user;
my $date = RT::Date->new(RT->SystemUser);
is($date->Unix, 0, "new date returns 0 in Unix format");
is($date->Get, '1970-01-01 00:00:00', "default is ISO format");
- is($date->Get(Format =>'SomeBadFormat'),
- '1970-01-01 00:00:00',
- "don't know format, return ISO format");
+ warning_like {
+ is($date->Get(Format =>'SomeBadFormat'),
+ '1970-01-01 00:00:00',
+ "don't know format, return ISO format");
+ } qr/Invalid date formatter/;
is($date->Get(Format =>'W3CDTF'),
'1970-01-01T00:00:00Z',
"W3CDTF format with defaults");
commit f258e65879c8c254c907d7d68c706d5fcea17486
Author: Thomas Sibley <trs at bestpractical.com>
Date: Thu Nov 17 15:21:10 2011 -0500
Require valid names for the format methods called by LocalizedDateTime
This only matches valid method names, so you can still shoot yourself in
the foot by passing something like time_format_bogus. There are too
many aliases in DateTime::Locale::Base to implement better sanity
checks.
This resolves part of CVE-2011-4458.
diff --git a/lib/RT/Date.pm b/lib/RT/Date.pm
index d6283bd..7951921 100644
--- a/lib/RT/Date.pm
+++ b/lib/RT/Date.pm
@@ -679,15 +679,19 @@ sub LocalizedDateTime
my %args = ( Date => 1,
Time => 1,
Timezone => '',
- DateFormat => 'date_format_full',
- TimeFormat => 'time_format_medium',
+ DateFormat => '',
+ TimeFormat => '',
AbbrDay => 1,
AbbrMonth => 1,
@_,
);
- my $date_format = $args{'DateFormat'};
- my $time_format = $args{'TimeFormat'};
+ # Require valid names for the format methods
+ my $date_format = $args{DateFormat} =~ /^\w+$/
+ ? $args{DateFormat} : 'date_format_full';
+
+ my $time_format = $args{TimeFormat} =~ /^\w+$/
+ ? $args{TimeFormat} : 'time_format_medium';
my $formatter = $self->LocaleObj;
$date_format = $formatter->$date_format;
commit 65ff771972e8973145fc4132dc459a0a3b53ad69
Author: Thomas Sibley <trs at bestpractical.com>
Date: Thu Nov 17 17:30:08 2011 -0500
Validate the requested link types when graphing relationships
Otherwise you can call bogus methods on the ticket that don't result in
an RT::Links object.
This resolves part of CVE-2011-4458.
diff --git a/lib/RT/Graph/Tickets.pm b/lib/RT/Graph/Tickets.pm
index 740e372..800ab40 100644
--- a/lib/RT/Graph/Tickets.pm
+++ b/lib/RT/Graph/Tickets.pm
@@ -278,6 +278,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,
commit 04a9551f9a6a4a8042dc30911133ad652a79c69b
Author: Thomas Sibley <trs at bestpractical.com>
Date: Fri Jan 6 11:07:13 2012 -0500
Explicitly override any Graph parameter passed into RT::Graph::Tickets
Specifying a defined Graph argument to RT::Graph::Tickets->TicketLinks
is only used internally when it is called recursively. Since Graph is
expected to be an existing GraphViz object if defined, it never makes
sense to start with anything but an undefined Graph parameter.
This prevents a user-supplied Graph parameter from having ->add_node
called on it. Since the Graph parameter could contain a Perl package
name, it previously provided a means to call to ->add_node on arbitrary
Perl packages already loaded into memory. While of unlikely utility,
there's no reason to allow such behaviour.
Fixes part of CVE-2011-4458.
diff --git a/share/html/Ticket/Graphs/Elements/ShowGraph b/share/html/Ticket/Graphs/Elements/ShowGraph
index 1d905c7..f4c07d5 100644
--- a/share/html/Ticket/Graphs/Elements/ShowGraph
+++ b/share/html/Ticket/Graphs/Elements/ShowGraph
@@ -66,6 +66,7 @@ $ARGS{'id'} = $id = $ticket->id;
require RT::Graph::Tickets;
my $graph = RT::Graph::Tickets->TicketLinks(
%ARGS,
+ Graph => undef,
Ticket => $ticket,
);
</%INIT>
diff --git a/share/html/Ticket/Graphs/dhandler b/share/html/Ticket/Graphs/dhandler
index a1dfebe..1335ed5 100644
--- a/share/html/Ticket/Graphs/dhandler
+++ b/share/html/Ticket/Graphs/dhandler
@@ -65,6 +65,7 @@ unless ( $ticket->id ) {
require RT::Graph::Tickets;
my $graph = RT::Graph::Tickets->TicketLinks(
%ARGS,
+ Graph => undef,
Ticket => $ticket,
);
commit fbef48d9f2271c87391c459477da1cb77d8a15b2
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Mon Mar 26 13:35:01 2012 -0400
Ensure that all joins through CachedGroupMembers limits to non-disabled rows
When a group becomes disabled in RT, we mark all CGM rows that existed
because of that group as 'Disabled'. Unfortunately, many joins through
CGM neglected to take the Disabled column into account, leading to users
possibly having rights that they should not, due to having them by way
of a disabled group.
This addresses CVE-2011-4459.
diff --git a/lib/RT/ACL.pm b/lib/RT/ACL.pm
index 00af5e9..486ac52 100644
--- a/lib/RT/ACL.pm
+++ b/lib/RT/ACL.pm
@@ -182,6 +182,9 @@ sub LimitToPrincipal {
ALIAS2 => $cgm,
FIELD2 => 'GroupId'
);
+ $self->Limit( ALIAS => $cgm,
+ FIELD => 'Disabled',
+ VALUE => 0 );
$self->Limit( ALIAS => $cgm,
FIELD => 'MemberId',
OPERATOR => '=',
diff --git a/lib/RT/Groups.pm b/lib/RT/Groups.pm
index 6a5b510..dd40e60 100644
--- a/lib/RT/Groups.pm
+++ b/lib/RT/Groups.pm
@@ -234,6 +234,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'};
return $members;
}
@@ -261,6 +263,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/lib/RT/Tickets.pm b/lib/RT/Tickets.pm
index ffef442..7339110 100644
--- a/lib/RT/Tickets.pm
+++ b/lib/RT/Tickets.pm
@@ -1097,6 +1097,12 @@ sub _GroupMembersJoin {
FIELD2 => 'GroupId',
ENTRYAGGREGATOR => 'AND',
);
+ $self->SUPER::Limit(
+ $args{'Left'} ? (LEFTJOIN => $alias) : (),
+ ALIAS => $alias,
+ FIELD => 'Disabled',
+ VALUE => 0,
+ );
$self->{'_sql_group_members_aliases'}{ $args{'GroupsAlias'} } = $alias
unless $args{'New'};
@@ -1261,6 +1267,12 @@ sub _WatcherMembershipLimit {
FIELD2 => 'id'
);
+ $self->Limit(
+ ALIAS => $groupmembers,
+ FIELD => 'Disabled',
+ VALUE => 0,
+ );
+
$self->Join(
ALIAS1 => $memberships,
FIELD1 => 'MemberId',
@@ -1268,6 +1280,13 @@ sub _WatcherMembershipLimit {
FIELD2 => 'id'
);
+ $self->Limit(
+ ALIAS => $memberships,
+ FIELD => 'Disabled',
+ VALUE => 0,
+ );
+
+
$self->_CloseParen;
}
diff --git a/lib/RT/User.pm b/lib/RT/User.pm
index cbc1a8f..1786ee1 100644
--- a/lib/RT/User.pm
+++ b/lib/RT/User.pm
@@ -1406,6 +1406,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");
diff --git a/lib/RT/Users.pm b/lib/RT/Users.pm
index ba56a38..19af4da 100644
--- a/lib/RT/Users.pm
+++ b/lib/RT/Users.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',
@@ -266,6 +269,11 @@ sub _JoinGroupMembers
ALIAS2 => $principals,
FIELD2 => 'id'
);
+ $self->Limit(
+ ALIAS => $group_members,
+ FIELD => 'Disabled',
+ VALUE => 0,
+ ) if $args{'IncludeSubgroupMembers'};
return $group_members;
}
@@ -284,6 +292,11 @@ sub _JoinGroups
ALIAS2 => $group_members,
FIELD2 => 'GroupId'
);
+ $self->Limit(
+ ALIAS => $groups,
+ FIELD => 'Disabled',
+ VALUE => 0,
+ );
return $groups;
}
diff --git a/t/web/owner_disabled_group_19221.t b/t/web/owner_disabled_group_19221.t
index f64af90..2664c5b 100644
--- a/t/web/owner_disabled_group_19221.t
+++ b/t/web/owner_disabled_group_19221.t
@@ -59,7 +59,132 @@ diag "user from disabled group DOESN'T shows up in create form";
my $input = $form->find_input('Owner');
is $input->value, RT->Nobody->Id, 'correct owner selected';
ok((not scalar grep { $_ == $user->Id } $input->possible_values), 'user from disabled group is NOT in dropdown');
+ ($ok, $msg) = $group->SetDisabled(0);
+ ok($ok, $msg);
+}
+
+
+
+diag "Put us in a nested group";
+my $super = RT::Group->new(RT->SystemUser);
+($ok, $msg) = $super->CreateUserDefinedGroup(Name => 'Supergroup');
+ok($ok, $msg);
+
+($ok, $msg) = $super->AddMember( $group->PrincipalId );
+ok($ok, $msg);
+
+ok( RT::Test->set_rights({
+ Principal => $super,
+ Object => $queue,
+ Right => [qw(OwnTicket)]
+}), 'set rights');
+
+
+diag "Disable the middle group";
+{
+ ($ok, $msg) = $group->SetDisabled(1);
+ ok($ok, "Disabled group: $msg");
+
+ $m->get_ok('/', 'open home page');
+ $m->form_name('CreateTicketInQueue');
+ $m->select( 'Queue', $queue->id );
+ $m->submit;
+
+ $m->content_contains('Create a new ticket', 'opened create ticket page');
+ my $form = $m->form_name('TicketCreate');
+ my $input = $form->find_input('Owner');
+ is $input->value, RT->Nobody->Id, 'correct owner selected';
+ ok((not scalar grep { $_ == $user->Id } $input->possible_values), 'user from disabled group is NOT in dropdown');
+ ($ok, $msg) = $group->SetDisabled(0);
+ ok($ok, "Re-enabled group: $msg");
}
+diag "Disable the top group";
+{
+ ($ok, $msg) = $super->SetDisabled(1);
+ ok($ok, "Disabled supergroup: $msg");
+
+ $m->get_ok('/', 'open home page');
+ $m->form_name('CreateTicketInQueue');
+ $m->select( 'Queue', $queue->id );
+ $m->submit;
+
+ $m->content_contains('Create a new ticket', 'opened create ticket page');
+ my $form = $m->form_name('TicketCreate');
+ my $input = $form->find_input('Owner');
+ is $input->value, RT->Nobody->Id, 'correct owner selected';
+ ok((not scalar grep { $_ == $user->Id } $input->possible_values), 'user from disabled group is NOT in dropdown');
+ ($ok, $msg) = $super->SetDisabled(0);
+ ok($ok, "Re-enabled supergroup: $msg");
+}
+
+
+diag "Check WithMember and WithoutMember recursively";
+{
+ my $with = RT::Groups->new( RT->SystemUser );
+ $with->WithMember( PrincipalId => $user->PrincipalObj->Id, Recursively => 1 );
+ $with->Limit( FIELD => 'domain', OPERATOR => '=', VALUE => 'UserDefined' );
+ is_deeply(
+ [map {$_->Name} @{$with->ItemsArrayRef}],
+ ['Disabled Group','Supergroup'],
+ "Get expected recursive memberships",
+ );
+
+ my $without = RT::Groups->new( RT->SystemUser );
+ $without->WithoutMember( PrincipalId => $user->PrincipalObj->Id, Recursively => 1 );
+ $without->Limit( FIELD => 'domain', OPERATOR => '=', VALUE => 'UserDefined' );
+ is_deeply(
+ [map {$_->Name} @{$without->ItemsArrayRef}],
+ [],
+ "And not a member of no groups",
+ );
+
+ ($ok, $msg) = $super->SetDisabled(1);
+ ok($ok, "Disabled supergroup: $msg");
+ $with->RedoSearch;
+ $without->RedoSearch;
+ is_deeply(
+ [map {$_->Name} @{$with->ItemsArrayRef}],
+ ['Disabled Group'],
+ "Recursive check only contains subgroup",
+ );
+ is_deeply(
+ [map {$_->Name} @{$without->ItemsArrayRef}],
+ [],
+ "Doesn't find the currently disabled group",
+ );
+ ($ok, $msg) = $super->SetDisabled(0);
+ ok($ok, "Re-enabled supergroup: $msg");
+
+ ($ok, $msg) = $group->SetDisabled(1);
+ ok($ok, "Disabled intermediate group: $msg");
+ $with->RedoSearch;
+ $without->RedoSearch;
+ is_deeply(
+ [map {$_->Name} @{$with->ItemsArrayRef}],
+ [],
+ "Recursive check finds no groups",
+ );
+ is_deeply(
+ [map {$_->Name} @{$without->ItemsArrayRef}],
+ ['Supergroup'],
+ "Now not a member of the supergroup",
+ );
+ ($ok, $msg) = $group->SetDisabled(0);
+ ok($ok, "Re-enabled intermediate group: $msg");
+}
+
+diag "Check MemberOfGroup";
+{
+ ($ok, $msg) = $group->SetDisabled(1);
+ ok($ok, "Disabled intermediate group: $msg");
+ my $users = RT::Users->new(RT->SystemUser);
+ $users->MemberOfGroup($super->PrincipalObj->id);
+ is($users->Count, 0, "Supergroup claims no members");
+ ($ok, $msg) = $group->SetDisabled(0);
+ ok($ok, "Re-enabled intermediate group: $msg");
+}
+
+
undef $m;
done_testing;
commit 312199f66c840c444c6414815dcc186c6653278e
Author: Kevin Falcone <falcone at bestpractical.com>
Date: Fri Mar 30 18:54:52 2012 -0400
Terminate the request if there isn't a CustomField or Context Argument
Make sure we return a valid JSON object so that parsers (like the test
suite) don't have to deal with incomplete output.
diff --git a/share/html/Helpers/Autocomplete/CustomFieldValues b/share/html/Helpers/Autocomplete/CustomFieldValues
index 65e5170..69c8f93 100644
--- a/share/html/Helpers/Autocomplete/CustomFieldValues
+++ b/share/html/Helpers/Autocomplete/CustomFieldValues
@@ -52,6 +52,14 @@
# Only autocomplete the last value
my $term = (split /\n/, $ARGS{term} || '')[-1];
+my $abort = sub {
+ $r->content_type('application/json');
+ $m->out(JSON::to_json( [] ));
+ $m->abort;
+};
+
+$abort->() unless $ARGS{ContextType} and $ARGS{ContextId};
+
my $CustomField;
for my $k ( keys %ARGS ) {
next unless $k =~ /^Object-.*?-\d*-CustomField-(\d+)-Values?$/;
@@ -59,9 +67,10 @@ for my $k ( keys %ARGS ) {
last;
}
-$m->abort unless $CustomField;
+$abort->() unless $CustomField;
my $CustomFieldObj = RT::CustomField->new( $session{'CurrentUser'} );
$CustomFieldObj->Load( $CustomField );
+$abort->() unless $CustomFieldObj->Id;
my $values = $CustomFieldObj->Values;
$values->Limit(
commit 86dc0486708f5b778b20c3a30c138beb0cf5e489
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Mon Apr 9 15:56:35 2012 -0400
Load and Validate Custom Field Context Objects
We now require that you pass a ContextId in the AutoComplete URL and we
use that Id to load an object of the appropriate type and check that the
object is actually related to the Custom Field you're looking at (this
prevents you from using Queue A to look at valuds for Custom Field B
which is only applied to Queue C, or using a Class of id 8 to simulate a
Queue of id 8).
Since you might use the Context Object to gain rights to load a
CustomField, we can't just load a random Custom Field and ask "is this a
context object?" which is why this code loads a Custom Field as the
system user and then uses that privileged object to validate the context
object before dropping privileges and loading the CO and CF as the
current user.
diff --git a/lib/RT/CustomField.pm b/lib/RT/CustomField.pm
index c5436da..1120840 100644
--- a/lib/RT/CustomField.pm
+++ b/lib/RT/CustomField.pm
@@ -883,7 +883,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 {
my $self = shift;
diff --git a/share/html/Elements/EditCustomFieldAutocomplete b/share/html/Elements/EditCustomFieldAutocomplete
index a7576eb..0cef26f 100644
--- a/share/html/Elements/EditCustomFieldAutocomplete
+++ b/share/html/Elements/EditCustomFieldAutocomplete
@@ -52,7 +52,7 @@
var id = '<% $name . '-Values' %>';
id = id.replace(/:/g,'\\:');
jQuery('#'+id).autocomplete( {
- source: "<%RT->Config->Get('WebPath')%>/Helpers/Autocomplete/CustomFieldValues?<% $name . '-Values' %>",
+ source: "<%RT->Config->Get('WebPath')%>/Helpers/Autocomplete/CustomFieldValues?<% $Context |n %><% $name . '-Values' %>",
focus: function () {
// prevent value inserted on focus
return false;
@@ -76,7 +76,7 @@ jQuery('#'+id).autocomplete( {
var id = '<% $name . '-Value' %>';
id = id.replace(/:/g,'\\:');
jQuery('#'+id).autocomplete( {
- source: "<%RT->Config->Get('WebPath')%>/Helpers/Autocomplete/CustomFieldValues?<% $name . '-Value' %>",
+ source: "<%RT->Config->Get('WebPath')%>/Helpers/Autocomplete/CustomFieldValues?<% $Context |n %><% $name . '-Value' %>",
}
);
% }
@@ -92,6 +92,11 @@ if ( $Multiple and $Values ) {
$Default .= $value->Content ."\n";
}
}
+my $Context = "";
+if ($CustomField->ContextObject) {
+ $Context .= "ContextId=" . $CustomField->ContextObject->Id . "&";
+ $Context .= "ContextType=". ref($CustomField->ContextObject) . "&";
+}
</%INIT>
<%ARGS>
$CustomField => undef
diff --git a/share/html/Helpers/Autocomplete/CustomFieldValues b/share/html/Helpers/Autocomplete/CustomFieldValues
index 69c8f93..fb24c49 100644
--- a/share/html/Helpers/Autocomplete/CustomFieldValues
+++ b/share/html/Helpers/Autocomplete/CustomFieldValues
@@ -58,7 +58,10 @@ my $abort = sub {
$m->abort;
};
-$abort->() unless $ARGS{ContextType} and $ARGS{ContextId};
+unless ( exists $ARGS{ContextType} and exists $ARGS{ContextId} ) {
+ RT->Logger->debug("No context provided");
+ $abort->();
+}
my $CustomField;
for my $k ( keys %ARGS ) {
@@ -67,10 +70,38 @@ for my $k ( keys %ARGS ) {
last;
}
-$abort->() unless $CustomField;
+unless ( $CustomField ) {
+ RT->Logger->debug("No CustomField provided");
+ $abort->();
+}
+
+my $SystemCustomFieldObj = RT::CustomField->new( RT->SystemUser );
+my ($id, $msg) = $SystemCustomFieldObj->LoadById( $CustomField ) ;
+unless ( $id ) {
+ RT->Logger->debug("Invalid CustomField provided: $msg");
+ $abort->();
+}
+
+my $context_object = $SystemCustomFieldObj->LoadContextObject(
+ $ARGS{ContextType}, $ARGS{ContextId} );
+$abort->() unless $context_object;
+
my $CustomFieldObj = RT::CustomField->new( $session{'CurrentUser'} );
-$CustomFieldObj->Load( $CustomField );
-$abort->() unless $CustomFieldObj->Id;
+if ( $SystemCustomFieldObj->ValidateContextObject($context_object) ) {
+ # drop our privileges that came from calling LoadContextObject as the System User
+ $context_object->new($session{'CurrentUser'});
+ $context_object->LoadById($ARGS{ContextId});
+ $CustomFieldObj->SetContextObject( $context_object );
+} else {
+ RT->Logger->debug("Invalid Context Object ".$context_object->id." for Custom Field ".$SystemCustomFieldObj->id);
+ $abort->();
+}
+
+($id, $msg) = $CustomFieldObj->LoadById( $CustomField );
+unless ( $CustomFieldObj->Name ) {
+ RT->Logger->debug("Current User cannot see this Custom Field, terminating");
+ $abort->();
+}
my $values = $CustomFieldObj->Values;
$values->Limit(
commit bb24a9f477d792ed77ecb8bf1bc29ae958734297
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Mon Apr 9 16:00:33 2012 -0400
When loading custom fields by queue, default the context object accordingly
diff --git a/lib/RT/CustomField.pm b/lib/RT/CustomField.pm
index 1120840..10b3ad9 100644
--- a/lib/RT/CustomField.pm
+++ b/lib/RT/CustomField.pm
@@ -465,10 +465,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
commit 3929c48b545f5d0245a31b7c61ee90bed45549be
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Mon Apr 9 18:29:46 2012 -0400
Set context objects on CFs explicitly whenever possible
diff --git a/lib/RT/Action/CreateTickets.pm b/lib/RT/Action/CreateTickets.pm
index 32b2bc0..3504feb 100644
--- a/lib/RT/Action/CreateTickets.pm
+++ b/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/lib/RT/Class.pm b/lib/RT/Class.pm
index 0b68f86..4c5cbb2 100644
--- a/lib/RT/Class.pm
+++ b/lib/RT/Class.pm
@@ -275,6 +275,7 @@ sub ArticleCustomFields {
my $cfs = RT::CustomFields->new( $self->CurrentUser );
if ( $self->CurrentUserHasRight('SeeClass') ) {
+ $cfs->SetContextObject( $self );
$cfs->LimitToGlobalOrObjectId( $self->Id );
$cfs->LimitToLookupType( RT::Article->CustomFieldLookupType );
$cfs->ApplySortOrder;
diff --git a/lib/RT/CustomField.pm b/lib/RT/CustomField.pm
index 10b3ad9..3e2f5bc 100644
--- a/lib/RT/CustomField.pm
+++ b/lib/RT/CustomField.pm
@@ -1767,6 +1767,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")
@@ -1784,6 +1785,7 @@ sub BasedOnObj {
my $self = shift;
my $obj = RT::CustomField->new( $self->CurrentUser );
+ $obj->SetContextObject( $self->ContextObject );
if ( $self->BasedOn ) {
$obj->Load( $self->BasedOn );
}
diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index c880115..878d074 100644
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -1355,6 +1355,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 );
@@ -2168,6 +2169,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");
diff --git a/lib/RT/ObjectCustomField.pm b/lib/RT/ObjectCustomField.pm
index 45286ed..8e13366 100644
--- a/lib/RT/ObjectCustomField.pm
+++ b/lib/RT/ObjectCustomField.pm
@@ -137,7 +137,19 @@ Returns the CustomField Object which has the id returned by CustomField
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/lib/RT/Queue.pm b/lib/RT/Queue.pm
index 9ff2f9e..7fd70ef 100644
--- a/lib/RT/Queue.pm
+++ b/lib/RT/Queue.pm
@@ -692,6 +692,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;
diff --git a/lib/RT/Shredder/Queue.pm b/lib/RT/Shredder/Queue.pm
index 8ee1094..79b67d1 100644
--- a/lib/RT/Shredder/Queue.pm
+++ b/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/lib/RT/Ticket.pm b/lib/RT/Ticket.pm
index 954015d..1b00d4d 100644
--- a/lib/RT/Ticket.pm
+++ b/lib/RT/Ticket.pm
@@ -3625,7 +3625,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/lib/RT/Transaction.pm b/lib/RT/Transaction.pm
index 04a74f2..9ea4b42 100644
--- a/lib/RT/Transaction.pm
+++ b/lib/RT/Transaction.pm
@@ -708,6 +708,7 @@ sub BriefDescription {
if ( $self->Field ) {
my $cf = RT::CustomField->new( $self->CurrentUser );
+ $cf->SetContextObject( $self->Object );
$cf->Load( $self->Field );
$field = $cf->Name();
$field = $self->loc('a custom field') if !defined($field);
@@ -1200,6 +1201,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/share/html/Admin/Elements/EditCustomFields b/share/html/Admin/Elements/EditCustomFields
index 91d5cff..8226390 100755
--- a/share/html/Admin/Elements/EditCustomFields
+++ b/share/html/Admin/Elements/EditCustomFields
@@ -128,6 +128,7 @@ if ( $MoveCustomFieldDown ) { {
if ( $UpdateCFs ) {
foreach my $cf_id ( @AddCustomField ) {
my $CF = RT::CustomField->new( $session{'CurrentUser'} );
+ $CF->SetContextObject( $Object );
$CF->Load( $cf_id );
unless ( $CF->id ) {
push @results, loc("Couldn't load CustomField #[_1]", $cf_id);
@@ -138,6 +139,7 @@ if ( $UpdateCFs ) {
}
foreach my $cf_id ( @RemoveCustomField ) {
my $CF = RT::CustomField->new( $session{'CurrentUser'} );
+ $CF->SetContextObject( $Object );
$CF->Load( $cf_id );
unless ( $CF->id ) {
push @results, loc("Couldn't load CustomField #[_1]", $cf_id);
@@ -153,6 +155,7 @@ $m->callback(CallbackName => 'UpdateExtraFields', Results => \@results, Object =
my $applied_cfs = RT::CustomFields->new( $session{'CurrentUser'} );
$applied_cfs->LimitToLookupType($lookup);
$applied_cfs->LimitToGlobalOrObjectId($id);
+$applied_cfs->SetContextObject( $Object );
$applied_cfs->ApplySortOrder;
my $not_applied_cfs = RT::CustomFields->new( $session{'CurrentUser'} );
diff --git a/share/html/Articles/Article/Edit.html b/share/html/Articles/Article/Edit.html
index 8c6e259..b418e06 100644
--- a/share/html/Articles/Article/Edit.html
+++ b/share/html/Articles/Article/Edit.html
@@ -157,6 +157,7 @@ elsif ( $id eq 'new' ) {
my $cfid = $1;
my $cf = RT::CustomField->new( $session{'CurrentUser'} );
+ $cf->SetContextObject( $ArticleObj );
$cf->Load( $cfid );
unless ( $cf->id ) {
$RT::Logger->error( "Couldn't load custom field #". $cfid );
commit 08754c08ae211c24cdba5b8390883f65578efc95
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Mon Apr 9 18:36:10 2012 -0400
Reuse the same custom field object for checking for DateTime type
This solves a subtle bug when ->Load is not sufficient, but
_CustomFieldDecipher's logic to limit to one queue is. It also ensures
that the same ContextObject is preserved.
diff --git a/lib/RT/Tickets.pm b/lib/RT/Tickets.pm
index 693338b..f86786e 100644
--- a/lib/RT/Tickets.pm
+++ b/lib/RT/Tickets.pm
@@ -1607,11 +1607,8 @@ sub _CustomFieldLimit {
$self->_CloseParen;
}
else {
- my $cf = RT::CustomField->new( $self->CurrentUser );
- $cf->Load($field);
-
# need special treatment for Date
- if ( $cf->Type eq 'DateTime' && $op eq '=' ) {
+ if ( $cf and $cf->Type eq 'DateTime' and $op eq '=' ) {
if ( $value =~ /:/ ) {
# there is time speccified.
commit 69178f9fc6ce3aecfc827987d81ba6fc92a5e96e
Merge: 37658c2 bb35edd
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Mon Apr 9 20:29:14 2012 -0400
Merge branch 'security/4.0/vulnerable-passwords' into security/4.0-trunk
commit b9a5e5f9b8c14ea97286484d02827bfd89169042
Merge: 69178f9 3141f16
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Mon Apr 9 20:29:32 2012 -0400
Merge branch 'security/4.0/escape-flags' into security/4.0-trunk
Conflicts:
share/html/Admin/Tools/Shredder/Elements/Error/NoStorage
commit 29d4827b4b5f0060bc2e76f564a9a26d8523e226
Merge: b9a5e5f c36e510
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Mon Apr 9 20:29:57 2012 -0400
Merge branch 'security/4.0/mobile-xss' into security/4.0-trunk
Conflicts:
share/html/m/ticket/create
diff --cc share/html/m/ticket/create
index 6232e87,a19e68e..b42787d
--- a/share/html/m/ticket/create
+++ b/share/html/m/ticket/create
@@@ -252,7 -264,7 +253,7 @@@ if ((!exists $ARGS{'AddMoreAttach'}) an
<%perl>
$showrows->(
- loc("Subject") => '<input type="text" name="Subject" size="30" maxsize="200" value="'.($ARGS{Subject} || '').'" />');
- loc("Subject") => '<input name="Subject" size="30" maxsize="200" value="'.$escape->($ARGS{Subject} || '').'" />');
++ loc("Subject") => '<input type="text" name="Subject" size="30" maxsize="200" value="'.$escape->($ARGS{Subject} || '').'" />');
</%perl>
<span class="content-label label"><%loc("Describe the issue below")%></span>
<& /Elements/MessageBox, exists $ARGS{Content} ? (Default => $ARGS{Content}, IncludeSignature => 0 ) : ( QuoteTransaction => $QuoteTransaction ), Height => 5 &>
@@@ -413,12 -428,12 +414,12 @@@ $showrows->
<%perl>
$showrows->(
- loc("Depends on") => '<input type="text" size="10" name="new-DependsOn" value="' . ($ARGS{'new-DependsOn'} || '' ). '" />',
- loc("Depended on by") => '<input type="text" size="10" name="DependsOn-new" value="' . ($ARGS{'DependsOn-new'} || '' ) . '" />',
- loc("Parents") => '<input type="text" size="10" name="new-MemberOf" value="' . ($ARGS{'new-MemberOf'} || '') . '" />',
- loc("Children") => '<input type="text" size="10" name="MemberOf-new" value="' . ($ARGS{'MemberOf-new'} || '') . '" />',
- loc("Refers to") => '<input type="text" size="10" name="new-RefersTo" value="' . ($ARGS{'new-RefersTo'} || '') . '" />',
- loc("Referred to by") => '<input type="text" size="10" name="RefersTo-new" value="' . ($ARGS{'RefersTo-new'} || ''). '" />'
- loc("Depends on") => '<input size="10" name="new-DependsOn" value="' . $escape->($ARGS{'new-DependsOn'} || '' ). '" />',
- loc("Depended on by") => '<input size="10" name="DependsOn-new" value="' . $escape->($ARGS{'DependsOn-new'} || '' ) . '" />',
- loc("Parents") => '<input size="10" name="new-MemberOf" value="' . $escape->($ARGS{'new-MemberOf'} || '') . '" />',
- loc("Children") => '<input size="10" name="MemberOf-new" value="' . $escape->($ARGS{'MemberOf-new'} || '') . '" />',
- loc("Refers to") => '<input size="10" name="new-RefersTo" value="' . $escape->($ARGS{'new-RefersTo'} || '') . '" />',
- loc("Referred to by") => '<input size="10" name="RefersTo-new" value="' . $escape->($ARGS{'RefersTo-new'} || ''). '" />'
++ loc("Depends on") => '<input type="text" size="10" name="new-DependsOn" value="' . $escape->($ARGS{'new-DependsOn'} || '' ). '" />',
++ loc("Depended on by") => '<input type="text" size="10" name="DependsOn-new" value="' . $escape->($ARGS{'DependsOn-new'} || '' ) . '" />',
++ loc("Parents") => '<input type="text" size="10" name="new-MemberOf" value="' . $escape->($ARGS{'new-MemberOf'} || '') . '" />',
++ loc("Children") => '<input type="text" size="10" name="MemberOf-new" value="' . $escape->($ARGS{'MemberOf-new'} || '') . '" />',
++ loc("Refers to") => '<input type="text" size="10" name="new-RefersTo" value="' . $escape->($ARGS{'new-RefersTo'} || '') . '" />',
++ loc("Referred to by") => '<input type="text" size="10" name="RefersTo-new" value="' . $escape->($ARGS{'RefersTo-new'} || ''). '" />'
);
</%perl>
commit e0ac46a7ef1cb3c61fa015ce3f2f8bcb870798b3
Merge: 29d4827 3718c5e
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Mon Apr 9 20:30:05 2012 -0400
Merge branch 'security/4.0/slash-l-xss' into security/4.0-trunk
commit e8c2f511c6fcc49f1e405e054cb9cedac027fe17
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Mon Apr 2 22:17:35 2012 -0400
Consistently escape all possibly suspect characters in JS strings
This resolves part of CVE-2011-2083.
diff --git a/lib/RT/Dashboard/Mailer.pm b/lib/RT/Dashboard/Mailer.pm
index 8878055..427f3d0 100644
--- a/lib/RT/Dashboard/Mailer.pm
+++ b/lib/RT/Dashboard/Mailer.pm
@@ -447,6 +447,9 @@ sub BuildEmail {
autohandler_name => '', # disable forced login and more
data_dir => $data_dir,
);
+ $mason->set_escape( h => \&RT::Interface::Web::EscapeUTF8 );
+ $mason->set_escape( u => \&RT::Interface::Web::EscapeURI );
+ $mason->set_escape( j => \&RT::Interface::Web::EscapeJS );
}
return $mason;
}
diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index c880115..04e4b87 100644
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -146,7 +146,24 @@ sub EscapeURI {
$$ref =~ s/([^a-zA-Z0-9_.!~*'()-])/uc sprintf("%%%02X", ord($1))/eg;
}
+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))
+ . "'";
+}
=head2 WebCanonicalizeInfo();
diff --git a/lib/RT/Interface/Web/Handler.pm b/lib/RT/Interface/Web/Handler.pm
index e7c8739..49d5329 100644
--- a/lib/RT/Interface/Web/Handler.pm
+++ b/lib/RT/Interface/Web/Handler.pm
@@ -117,6 +117,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);
}
diff --git a/share/html/Admin/Elements/EditRights b/share/html/Admin/Elements/EditRights
index 5ef7389..8aef203 100644
--- a/share/html/Admin/Elements/EditRights
+++ b/share/html/Admin/Elements/EditRights
@@ -110,13 +110,13 @@ for my $category (@$Principals) {
id="AddPrincipalForRights-<% lc $AddPrincipal %>" />
<script type="text/javascript">
jQuery(function() {
- jQuery("#AddPrincipalForRights-<% lc $AddPrincipal %>").keyup(function(){
+ jQuery("#AddPrincipalForRights-"+<% lc $AddPrincipal |n,j%>).keyup(function(){
toggle_addprincipal_validity(this, true);
});
% if (lc $AddPrincipal eq 'group') {
- jQuery("#AddPrincipalForRights-<% lc $AddPrincipal %>").autocomplete({
- source: "<% RT->Config->Get('WebPath')%>/Helpers/Autocomplete/Groups",
+ jQuery("#AddPrincipalForRights-"+<% lc $AddPrincipal |n,j%>).autocomplete({
+ source: <% RT->Config->Get('WebPath') |n,j%>+"/Helpers/Autocomplete/Groups",
select: addprincipal_onselect,
change: addprincipal_onchange
});
diff --git a/share/html/Admin/Elements/SelectNewGroupMembers b/share/html/Admin/Elements/SelectNewGroupMembers
index 27b6f70..63c305c 100755
--- a/share/html/Admin/Elements/SelectNewGroupMembers
+++ b/share/html/Admin/Elements/SelectNewGroupMembers
@@ -50,8 +50,8 @@
<input type="text" value="" name="<% $Name %>Users" id="<% $Name %>Users" /><br />
<script type="text/javascript">
jQuery(function(){
- jQuery("#<% $Name %>Users").autocomplete({
- source: "<% RT->Config->Get('WebPath')%>/Helpers/Autocomplete/Users?return=Name;privileged=1;exclude=<% $user_ids |u %>",
+ jQuery("#"+<% $Name |n,j%>+"Users").autocomplete({
+ source: <% RT->Config->Get('WebPath') |n,j%>+"/Helpers/Autocomplete/Users?return=Name;privileged=1;exclude="+<% $user_ids |n,u,j %>,
// Auto-submit once a user is chosen
select: function( event, ui ) {
jQuery(event.target).val(ui.item.value);
@@ -67,8 +67,8 @@ jQuery(function(){
<input type="text" value="" name="<% $Name %>Groups" id="<% $Name %>Groups" /><br />
<script type="text/javascript">
jQuery(function(){
- jQuery("#<% $Name %>Groups").autocomplete({
- source: "<% RT->Config->Get('WebPath')%>/Helpers/Autocomplete/Groups?exclude=<% $group_ids |u %>",
+ jQuery("#"+<% $Name |n,j%>+"Groups").autocomplete({
+ source: <% RT->Config->Get('WebPath') |n,j%>+"/Helpers/Autocomplete/Groups?exclude="+<% $group_ids |n,u,j %>,
// Auto-submit once a user is chosen
select: function( event, ui ) {
jQuery(event.target).val(ui.item.value);
diff --git a/share/html/Admin/Groups/index.html b/share/html/Admin/Groups/index.html
index da29703..a9603c1 100755
--- a/share/html/Admin/Groups/index.html
+++ b/share/html/Admin/Groups/index.html
@@ -57,7 +57,7 @@
<script type="text/javascript">
jQuery(function(){
jQuery("#autocomplete-GroupString").autocomplete({
- source: "<% RT->Config->Get('WebPath')%>/Helpers/Autocomplete/Groups",
+ source: <% RT->Config->Get('WebPath') |n,j%>+"/Helpers/Autocomplete/Groups",
// Auto-submit once a group is chosen
select: function( event, ui ) {
jQuery(event.target).val(ui.item.value);
diff --git a/share/html/Admin/Tools/Queries.html b/share/html/Admin/Tools/Queries.html
index 1bbf5b8..aa6d8f1 100644
--- a/share/html/Admin/Tools/Queries.html
+++ b/share/html/Admin/Tools/Queries.html
@@ -79,7 +79,7 @@ unless ($session{'CurrentUser'}->HasRight( Object=> $RT::System, Right => 'Super
<li>
<tt><% $request->{Path} %></tt> - <i><&|/l, sprintf('%.4f', $seconds) &>[_1]s</&></i>
- <a href="#" onclick="return hideshow('queries-<%$r%>');"><&|/l, $count &>Toggle [quant,_1,query,queries]</&></a>
+ <a href="#" onclick="return hideshow(<% "queries-$r" |n,j%>);"><&|/l, $count &>Toggle [quant,_1,query,queries]</&></a>
<table id="queries-<%$r%>" class="tablesorter hidden">
<thead>
<tr>
@@ -115,7 +115,7 @@ unless ($session{'CurrentUser'}->HasRight( Object=> $RT::System, Right => 'Super
<br><tt>[<% join(", ", @$b) %>]</tt>
% }
% }
- <a class="query-stacktrace-toggle" href="#" onclick="return hideshow('trace-<%$r%>-<%$s%>');"><&|/l &>Toggle stack trace</&></a>
+ <a class="query-stacktrace-toggle" href="#" onclick="return hideshow(<% "trace-$r-$s" |n,j%>);"><&|/l &>Toggle stack trace</&></a>
<pre id="trace-<%$r%>-<%$s%>" class="hidden"><% $trace %></pre>
</td>
</tr>
diff --git a/share/html/Admin/Users/index.html b/share/html/Admin/Users/index.html
index 178fd1c..19f1886 100755
--- a/share/html/Admin/Users/index.html
+++ b/share/html/Admin/Users/index.html
@@ -62,7 +62,7 @@
<script type="text/javascript">
jQuery(function(){
jQuery("#autocomplete-UserString").autocomplete({
- source: "<% RT->Config->Get('WebPath')%>/Helpers/Autocomplete/Users?return=Name",
+ source: <% RT->Config->Get('WebPath') |n,j%>+"/Helpers/Autocomplete/Users?return=Name",
// Auto-submit once a user is chosen
select: function( event, ui ) {
jQuery(event.target).val(ui.item.value);
diff --git a/share/html/Articles/Article/ExtractIntoClass.html b/share/html/Articles/Article/ExtractIntoClass.html
index 58b50d7..8e8c429 100644
--- a/share/html/Articles/Article/ExtractIntoClass.html
+++ b/share/html/Articles/Article/ExtractIntoClass.html
@@ -54,7 +54,7 @@
% my $Classes = RT::Classes->new($session{'CurrentUser'});
% $Classes->LimitToEnabled();
% while (my $Class = $Classes->Next) {
-<li><a href="ExtractIntoTopic.html?Ticket=<%$Ticket%>&Class=<%$Class->Id%>" onclick="document.getElementById('topics-<% $Class->Id %>').style.display = (document.getElementById('topics-<% $Class->Id %>').style.display == 'block') ? 'none' : 'block'; return false;"><%$Class->Name%></a>:
+<li><a href="ExtractIntoTopic.html?Ticket=<%$Ticket%>&Class=<%$Class->Id%>" onclick="document.getElementById('topics-'+<% $Class->Id |n,j%>).style.display = (document.getElementById('topics-'+<% $Class->Id |n,j%>).style.display == 'block') ? 'none' : 'block'; return false;"><%$Class->Name%></a>:
<%$Class->Description%>
<div id="topics-<%$Class->Id%>" style="display: none">
<form action="ExtractFromTicket.html">
diff --git a/share/html/Elements/ColumnMap b/share/html/Elements/ColumnMap
index 2e8506c..5c4220c 100644
--- a/share/html/Elements/ColumnMap
+++ b/share/html/Elements/ColumnMap
@@ -118,14 +118,16 @@ my $COLUMN_MAP = {
my $name = $_[1] || 'SelectedTickets';
my $checked = $m->request_args->{ $name .'All' }? 'checked="checked"': '';
- return \qq{<input type="checkbox" name="${name}All" value="1" $checked
- onclick="setCheckbox(this.form, '$name', this.checked)" />};
+ return \qq{<input type="checkbox" name="}, $name, \qq{All" value="1" $checked
+ onclick="setCheckbox(this.form, },
+ $m->interp->apply_escapes($name,'j'),
+ \qq{, this.checked)" />};
},
value => sub {
my $id = $_[0]->id;
my $name = $_[2] || 'SelectedTickets';
- return \qq{<input type="checkbox" name="$name" value="$id" checked="checked" />}
+ return \qq{<input type="checkbox" name="}, $name, \qq{" value="$id" checked="checked" />}
if $m->request_args->{ $name . 'All'};
my $arg = $m->request_args->{ $name };
@@ -136,7 +138,7 @@ my $COLUMN_MAP = {
elsif ( $arg ) {
$checked = 'checked="checked"' if $arg == $id;
}
- return \qq{<input type="checkbox" name="$name" value="$id" $checked />}
+ return \qq{<input type="checkbox" name="}, $name, \qq{" value="$id" $checked />}
},
},
RadioButton => {
diff --git a/share/html/Elements/EditCustomFieldAutocomplete b/share/html/Elements/EditCustomFieldAutocomplete
index a7576eb..7b45e2f 100644
--- a/share/html/Elements/EditCustomFieldAutocomplete
+++ b/share/html/Elements/EditCustomFieldAutocomplete
@@ -49,10 +49,10 @@
<textarea cols="<% $Cols %>" rows="<% $Rows %>" name="<% $name %>-Values" id="<% $name %>-Values" class="CF-<%$CustomField->id%>-Edit"><% $Default || '' %></textarea>
<script type="text/javascript">
-var id = '<% $name . '-Values' %>';
+var id = <% "$name-Values" |n,j%>;
id = id.replace(/:/g,'\\:');
jQuery('#'+id).autocomplete( {
- source: "<%RT->Config->Get('WebPath')%>/Helpers/Autocomplete/CustomFieldValues?<% $name . '-Values' %>",
+ source: <%RT->Config->Get('WebPath') |n,j%>+"/Helpers/Autocomplete/CustomFieldValues?"+<% "$name-Values" |n,u,j%>,
focus: function () {
// prevent value inserted on focus
return false;
@@ -73,10 +73,10 @@ jQuery('#'+id).autocomplete( {
% } else {
<input type="text" id="<% $name %>-Value" name="<% $name %>-Value" class="CF-<%$CustomField->id%>-Edit" value="<% $Default || '' %>"/>
<script type="text/javascript">
-var id = '<% $name . '-Value' %>';
+var id = <% "$name-Value" |n,j%>;
id = id.replace(/:/g,'\\:');
jQuery('#'+id).autocomplete( {
- source: "<%RT->Config->Get('WebPath')%>/Helpers/Autocomplete/CustomFieldValues?<% $name . '-Value' %>",
+ source: <%RT->Config->Get('WebPath')|n,j%>+"/Helpers/Autocomplete/CustomFieldValues?"+<% "$name-Value" |n,u,j%>,
}
);
% }
diff --git a/share/html/Elements/EditCustomFieldSelect b/share/html/Elements/EditCustomFieldSelect
index 0390ded..f7ccf00 100644
--- a/share/html/Elements/EditCustomFieldSelect
+++ b/share/html/Elements/EditCustomFieldSelect
@@ -55,7 +55,7 @@
% if (!$HideCategory and @category and not $CustomField->BasedOnObj->id) {
<script type="text/javascript" src="<%RT->Config->Get('WebPath')%>/NoAuth/js/cascaded.js"></script>
%# XXX - Hide this select from w3m?
- <select onchange="filter_cascade('<% $id %>-Values', this.value)" name="<% $id %>-Category" class="CF-<%$CustomField->id%>-Edit">
+ <select onchange="filter_cascade(<% "$id-Values" |n,j%>, this.value)" name="<% $id %>-Category" class="CF-<%$CustomField->id%>-Edit">
<option value=""<% !$selected && qq[ selected="selected"] |n %>><&|/l&>-</&></option>
% foreach my $cat (@category) {
% my ($depth, $name) = @$cat;
@@ -66,12 +66,12 @@
<script type="text/javascript" src="<%RT->Config->Get('WebPath')%>/NoAuth/js/cascaded.js"></script>
<script type="text/javascript"><!--
jQuery( function () {
- var basedon = document.getElementById('<% $NamePrefix . $CustomField->BasedOnObj->id %>-Values');
+ var basedon = document.getElementById(<% $NamePrefix . $CustomField->BasedOnObj->id . "-Values" |n,j%>);
if (basedon != null) {
var oldchange = basedon.onchange;
basedon.onchange = function () {
filter_cascade(
- '<% $id %>-Values',
+ <% "$id-Values" |n,j%>,
basedon.value,
1
);
diff --git a/share/html/Elements/HeaderJavascript b/share/html/Elements/HeaderJavascript
index 2abe5a6..2d2baed 100644
--- a/share/html/Elements/HeaderJavascript
+++ b/share/html/Elements/HeaderJavascript
@@ -60,14 +60,14 @@ $onload => undef
<script type="text/javascript"><!--
jQuery( loadTitleBoxStates );
% if ( $focus ) {
- jQuery(function () { focusElementById('<% $focus %>') });
+ jQuery(function () { focusElementById(<% $focus |n,j%>) });
% }
% if ( $onload ) {
jQuery( <% $onload |n %> );
% }
% if ( $RichText and RT->Config->Get('MessageBoxRichText', $session{'CurrentUser'})) {
- jQuery().ready(function () { ReplaceAllTextareas('<%$m->request_args->{'CKeditorEncoded'} || 0 %>') });
+ jQuery().ready(function () { ReplaceAllTextareas(<%$m->request_args->{'CKeditorEncoded'} || 0 |n,j%>) });
% }
--></script>
<%ARGS>
diff --git a/share/html/Elements/RT__CustomField/ColumnMap b/share/html/Elements/RT__CustomField/ColumnMap
index c0e17f2..ecaa3b7 100644
--- a/share/html/Elements/RT__CustomField/ColumnMap
+++ b/share/html/Elements/RT__CustomField/ColumnMap
@@ -120,8 +120,10 @@ my $COLUMN_MAP = {
my $name = 'RemoveCustomField';
my $checked = $m->request_args->{ $name .'All' }? 'checked="checked"': '';
- return \qq{<input type="checkbox" name="${name}All" value="1" $checked
- onclick="setCheckbox(this.form, '$name', this.checked)" />};
+ return \qq{<input type="checkbox" name="}, $name, \qq{All" value="1" $checked
+ onclick="setCheckbox(this.form, },
+ $m->interp->apply_escapes($name,'j'),
+ \qq{, this.checked)" />};
},
value => sub {
my $id = $_[0]->id;
@@ -137,7 +139,7 @@ my $COLUMN_MAP = {
elsif ( $arg ) {
$checked = 'checked="checked"' if $arg == $id;
}
- return \qq{<input type="checkbox" name="$name" value="$id" $checked />}
+ return \qq{<input type="checkbox" name="}, $name, \qq{" value="$id" $checked />}
},
},
MoveCF => {
diff --git a/share/html/Elements/RT__Dashboard/ColumnMap b/share/html/Elements/RT__Dashboard/ColumnMap
index 701db53..a1af44f 100644
--- a/share/html/Elements/RT__Dashboard/ColumnMap
+++ b/share/html/Elements/RT__Dashboard/ColumnMap
@@ -111,7 +111,7 @@ my $COLUMN_MAP = {
}
}
- return \('<a href="'.$url.'">'.$frequency.'</a>');
+ return \'<a href="', $url, \'">', $frequency, \'</a>';
},
},
ShowURL => {
diff --git a/share/html/Elements/SelectOwnerAutocomplete b/share/html/Elements/SelectOwnerAutocomplete
index 374dd50..7e62cd9 100644
--- a/share/html/Elements/SelectOwnerAutocomplete
+++ b/share/html/Elements/SelectOwnerAutocomplete
@@ -78,7 +78,7 @@ my $query = $m->comp('/Elements/QueryString',
<script type="text/javascript">
jQuery(function() {
var cache = {};
- jQuery("#<% $Name %>").autocomplete({
+ jQuery("#"+<% $Name |n,j%>).autocomplete({
minLength: 2,
source: function(request, response) {
if ( request.term in cache ) {
@@ -86,7 +86,7 @@ my $query = $m->comp('/Elements/QueryString',
}
else {
jQuery.ajax({
- url: "<% RT->Config->Get('WebPath')%>/Helpers/Autocomplete/Owners?<% $query|n %>",
+ url: <% RT->Config->Get('WebPath')|n,j%>+"/Helpers/Autocomplete/Owners?"+<% $query|n,j %>,
dataType: "json",
data: request,
success: function( data ) {
diff --git a/share/html/Elements/ShowCustomFields b/share/html/Elements/ShowCustomFields
index 2f7fc24..38bebcf 100644
--- a/share/html/Elements/ShowCustomFields
+++ b/share/html/Elements/ShowCustomFields
@@ -114,12 +114,12 @@ my $print_value = sub {
my $vid = $value->id;
$m->out( '<div class="object_cf_value_include" id="object_cf_value_'. $vid .'">' );
$m->out( loc("See also:") );
- $m->out( '<a href="'. $value->IncludeContentForValue .'">' );
- $m->out( $value->IncludeContentForValue );
+ $m->out( '<a href="'. $m->interp->apply_escapes($value->IncludeContentForValue, 'h') .'">' );
+ $m->out( $m->interp->apply_escapes($value->IncludeContentForValue, 'h') );
$m->out( qq{</a></div>\n} );
- $m->out( qq{<script><!--\njQuery('#object_cf_value_$vid').load('} );
- $m->out( $value->IncludeContentForValue );
- $m->out( qq{');\n--></script>\n} );
+ $m->out( qq{<script><!--\njQuery('#object_cf_value_$vid').load(} );
+ $m->out( $m->interp->apply_escapes($value->IncludeContentForValue, 'j') );
+ $m->out( qq{);\n--></script>\n} );
}
};
diff --git a/share/html/Elements/Submit b/share/html/Elements/Submit
index 6a45714..da38820 100755
--- a/share/html/Elements/Submit
+++ b/share/html/Elements/Submit
@@ -52,10 +52,10 @@ id="<%$id%>"
>
<div class="extra-buttons">
% if ($CheckAll) {
- <input type="button" value="<%$CheckAllLabel%>" onclick="setCheckbox(this.form, <% length $CheckboxName ? qq{'$CheckboxName'} : length $CheckboxNameRegex ? $CheckboxNameRegex : q{''} %>, true);return false;" class="button" />
+ <input type="button" value="<%$CheckAllLabel%>" onclick="setCheckbox(this.form, <% $match %>, true);return false;" class="button" />
% }
% if ($ClearAll) {
- <input type="button" value="<%$ClearAllLabel%>" onclick="setCheckbox(this.form, <% length $CheckboxName ? qq{'$CheckboxName'} : length $CheckboxNameRegex ? $CheckboxNameRegex : q{''} %>, false);return false;" class="button" />
+ <input type="button" value="<%$ClearAllLabel%>" onclick="setCheckbox(this.form, <% $match %>, false);return false;" class="button" />
% }
% if ($Reset) {
<input type="reset" value="<%$ResetLabel%>" class="button" />
@@ -115,3 +115,13 @@ $ResetLabel => loc('Reset')
$SubmitId => undef
$id => undef
</%ARGS>
+<%init>
+my $match;
+if (length $CheckboxName) {
+ $match = $m->interp->apply_escapes($CheckboxName,'j');
+} elsif (length $CheckboxNameRegex) {
+ $match = $CheckboxNameRegex;
+} else {
+ $match = q{''};
+}
+</%init>
diff --git a/share/html/NoAuth/js/titlebox-state.js b/share/html/NoAuth/js/titlebox-state.js
index ac0c2f0..2d31ec3 100644
--- a/share/html/NoAuth/js/titlebox-state.js
+++ b/share/html/NoAuth/js/titlebox-state.js
@@ -46,7 +46,7 @@
%#
%# END BPS TAGGED BLOCK }}}
function createCookie(name,value,days) {
- var path = "<%RT->Config->Get('WebPath')%>" ? "<%RT->Config->Get('WebPath')%>" : "/";
+ var path = <%RT->Config->Get('WebPath')|n,j%> ? <%RT->Config->Get('WebPath')|n,j%> : "/";
if (days) {
var date = new Date();
diff --git a/share/html/NoAuth/js/userautocomplete.js b/share/html/NoAuth/js/userautocomplete.js
index 1608deb..3d15155 100644
--- a/share/html/NoAuth/js/userautocomplete.js
+++ b/share/html/NoAuth/js/userautocomplete.js
@@ -70,7 +70,7 @@ jQuery(function() {
continue;
var options = {
- source: "<% RT->Config->Get('WebPath')%>/Helpers/Autocomplete/Users"
+ source: <% RT->Config->Get('WebPath') |n,j%>+"/Helpers/Autocomplete/Users"
};
var queryargs = [];
diff --git a/share/html/NoAuth/js/util.js b/share/html/NoAuth/js/util.js
index b1a4a0b..d1f13c6 100644
--- a/share/html/NoAuth/js/util.js
+++ b/share/html/NoAuth/js/util.js
@@ -294,8 +294,8 @@ function ReplaceAllTextareas(encoded) {
textArea.parentNode.appendChild(typeField);
- CKEDITOR.replace(textArea.name,{width:'100%',height:'<% RT->Config->Get('MessageBoxRichTextHeight') %>'});
- CKEDITOR.basePath = "<%RT->Config->Get('WebPath')%>/NoAuth/RichText/";
+ CKEDITOR.replace(textArea.name,{width:'100%',height:<% RT->Config->Get('MessageBoxRichTextHeight') |n,j%>});
+ CKEDITOR.basePath = <%RT->Config->Get('WebPath')|n,j%>+"/NoAuth/RichText/";
jQuery("#" + textArea.name + "___Frame").addClass("richtext-editor");
}
diff --git a/share/html/Ticket/Elements/Bookmark b/share/html/Ticket/Elements/Bookmark
index 414d79b..c2d93aa 100644
--- a/share/html/Ticket/Elements/Bookmark
+++ b/share/html/Ticket/Elements/Bookmark
@@ -83,7 +83,7 @@ $Toggle => 0
</%ARGS>
<span class="toggle-bookmark-<% $id %>">
% my $url = RT->Config->Get('WebPath') ."/Helpers/Toggle/TicketBookmark?id=". $id;
-<a align="right" href="<% $url %>" onclick="jQuery('.toggle-bookmark-<% $id |n%>').load('<% $url |n %>'); return false;" >
+<a align="right" href="<% $url %>" onclick="jQuery('.toggle-bookmark-'+<% $id |n,j%>).load(<% $url |n,j %>); return false;" >
% if ( $bookmarked ) {
<img src="<% RT->Config->Get('WebPath') %>/NoAuth/images/star.gif" alt="<% loc('Remove Bookmark') %>" style="border-style: none" />
% } else {
diff --git a/share/html/Ticket/Elements/ClickToShowHistory b/share/html/Ticket/Elements/ClickToShowHistory
index 792fa06..cee2e60 100644
--- a/share/html/Ticket/Elements/ClickToShowHistory
+++ b/share/html/Ticket/Elements/ClickToShowHistory
@@ -47,7 +47,7 @@
%# END BPS TAGGED BLOCK }}}
<div id="deferred_ticket_history">
<& /Widgets/TitleBoxStart, title => 'History' &>
- <a href="<% $url %>" onclick="jQuery('#deferred_ticket_history').text('<% loc('Loading...') %>').load('<% $url |n %>'); return false;" ><% loc('Show ticket history') %></a>
+ <a href="<% $url %>" onclick="jQuery('#deferred_ticket_history').text(<% loc('Loading...') |n,j%>).load(<% $url |n,j %>); return false;" ><% loc('Show ticket history') %></a>
<& /Widgets/TitleBoxEnd &>
</div>
<%ARGS>
diff --git a/share/html/Ticket/Elements/FoldStanzaJS b/share/html/Ticket/Elements/FoldStanzaJS
index 4c13b64..8ec7c6b 100644
--- a/share/html/Ticket/Elements/FoldStanzaJS
+++ b/share/html/Ticket/Elements/FoldStanzaJS
@@ -47,4 +47,4 @@
%# END BPS TAGGED BLOCK }}}
<span
class="message-stanza-folder closed"
- onclick="fold_message_stanza(this, '<%loc('Show quoted text')%>', '<%loc('Hide quoted text')%>');"><%loc('Show quoted text')%></span><br />\
+ onclick="fold_message_stanza(this, <%loc('Show quoted text') |n,j%>, <%loc('Hide quoted text') |n,j%>);"><%loc('Show quoted text')%></span><br />\
diff --git a/share/html/Ticket/Elements/ShowHistory b/share/html/Ticket/Elements/ShowHistory
index d655f94..b40586d 100755
--- a/share/html/Ticket/Elements/ShowHistory
+++ b/share/html/Ticket/Elements/ShowHistory
@@ -60,11 +60,12 @@ if ($ShowDisplayModes or $ShowTitle) {
if ($ShowDisplayModes) {
$titleright = '';
- my $open_all = $m->interp->apply_escapes( loc("Show all quoted text"), 'h' );
- my $close_all = $m->interp->apply_escapes( loc("Hide all quoted text"), 'h' );
+ my $open_all = $m->interp->apply_escapes( loc("Show all quoted text"), 'j' );
+ my $open_html = $m->interp->apply_escapes( loc("Show all quoted text"), 'h' );
+ my $close_all = $m->interp->apply_escapes( loc("Hide all quoted text"), 'j' );
$titleright .= '<a href="#" data-direction="open" '
- . qq{onclick='return toggle_all_folds(this, "$open_all", "$close_all");'}
- . ">$open_all</a> — ";
+ . qq{onclick="return toggle_all_folds(this, $open_all, $close_all);"}
+ . ">$open_html</a> — ";
if ($ShowHeaders) {
$titleright .= qq{<a href="$URIFile?id=} .
diff --git a/share/html/Ticket/Elements/UpdateCc b/share/html/Ticket/Elements/UpdateCc
index aee311c..3f789f0 100644
--- a/share/html/Ticket/Elements/UpdateCc
+++ b/share/html/Ticket/Elements/UpdateCc
@@ -61,8 +61,7 @@
class="onetime onetimecc"
type="checkbox"
% my $clean_addr = $txn_addresses{$addr}->format;
-% $clean_addr =~ s/'/\\'/g;
- onClick="checkboxToInput('UpdateCc', 'UpdateCc-<%$addr%>','<%$clean_addr%>' );"
+ onClick="checkboxToInput('UpdateCc', <% "UpdateCc-$addr" |n,j%>, <%$clean_addr|n,j%> );"
<% $ARGS{'UpdateCc-'.$addr} ? 'checked="checked"' : ''%> > <& /Elements/ShowUser, Address => $txn_addresses{$addr}&>
%}
</td></tr>
@@ -77,8 +76,7 @@
class="onetime onetimebcc"
type="checkbox"
% my $clean_addr = $txn_addresses{$addr}->format;
-% $clean_addr =~ s/'/\\'/g;
- onClick="checkboxToInput('UpdateBcc', 'UpdateBcc-<%$addr%>','<%$clean_addr%>' );"
+ onClick="checkboxToInput('UpdateBcc', <% "UpdateBcc-$addr" |n,j%>, <%$clean_addr|n,j%> );"
<% $ARGS{'UpdateBcc-'.$addr} ? 'checked="checked"' : ''%>>
<& /Elements/ShowUser, Address => $txn_addresses{$addr}&>
%}
diff --git a/share/html/Ticket/Graphs/Elements/EditGraphProperties b/share/html/Ticket/Graphs/Elements/EditGraphProperties
index beb67a2..b1fc1c3 100644
--- a/share/html/Ticket/Graphs/Elements/EditGraphProperties
+++ b/share/html/Ticket/Graphs/Elements/EditGraphProperties
@@ -151,7 +151,7 @@ my $class = '';
$class = 'class="hidden"' if $Level != 1 && !@Default;
</%INIT>
<% loc('Show Tickets Properties on [_1] level', $Level) %>
-(<small><a href="#" onclick="hideshow('<% $id %>'); return false;"><% loc('open/close') %></a></small>):
+(<small><a href="#" onclick="hideshow(<% $id |n,j%>); return false;"><% loc('open/close') %></a></small>):
<table id="<% $id %>" <% $class |n %>>
% while ( my ($group, $list) = (splice @Available, 0, 2) ) {
<tr><td><% loc($group) %>:</td><td>
diff --git a/share/html/Widgets/ComboBox b/share/html/Widgets/ComboBox
index 6d4e9f7..d4e4c2c 100644
--- a/share/html/Widgets/ComboBox
+++ b/share/html/Widgets/ComboBox
@@ -56,7 +56,7 @@ my $z_index = 9999;
<div id="<% $Name %>_Container" class="combobox <%$Class%>" style="z-index: <%$z_index--%>">
<input name="<% $Name %>" id="<% $Name %>" class="combo-text" value="<% $Default || '' %>" type="text" <% $Size ? "size='$Size'" : '' |n %> autocomplete="off" />
-<br style="display: none" /><span id="<% $Name %>_Button" class="combo-button">▼</span><select name="List-<% $Name %>" id="<% $Name %>_List" class="combo-list" onchange="ComboBox_SimpleAttach(this, this.form['<% $Name %>']); " size="<% $Rows %>">
+<br style="display: none" /><span id="<% $Name %>_Button" class="combo-button">▼</span><select name="List-<% $Name %>" id="<% $Name %>_List" class="combo-list" onchange="ComboBox_SimpleAttach(this, this.form[<% $Name |n,j%>]); " size="<% $Rows %>">
<option style="display: none" value="">-</option>
% foreach my $value (@Values) {
<option value="<%$value%>"><% $value%></option>
@@ -64,7 +64,7 @@ my $z_index = 9999;
</select>
</div>
<script language="javascript"><!--
-ComboBox_InitWith('<% $Name %>');
+ComboBox_InitWith(<% $Name |n,j %>);
//--></script>
</nobr>
<%ARGS>
diff --git a/share/html/Widgets/TitleBoxStart b/share/html/Widgets/TitleBoxStart
index e4b4b86..3936657 100755
--- a/share/html/Widgets/TitleBoxStart
+++ b/share/html/Widgets/TitleBoxStart
@@ -48,7 +48,7 @@
<div class="titlebox<% $class ? " $class " : '' %><% $rolledup ? " rolled-up" : ""%>" id="<% $id %>">
<div class="titlebox-title<% $title_class ? " $title_class" : ''%>">
% if ($hideable) {
- <span class="widget"><a href="#" onclick="return rollup('<%$tid%>');" title="Toggle visibility"></a></span>
+ <span class="widget"><a href="#" onclick="return rollup(<%$tid|n,j%>);" title="Toggle visibility"></a></span>
% }
<span class="left"><%
$title_href ? qq[<a href="$title_href">] : '' | n
diff --git a/share/html/m/ticket/show b/share/html/m/ticket/show
index afea61e..f71d1a4 100644
--- a/share/html/m/ticket/show
+++ b/share/html/m/ticket/show
@@ -186,18 +186,18 @@ my $print_value = sub {
}
$m->out('</a>') if defined $linked && length $linked;
- # This section automatically populates a<div with the "IncludeContentForValue" for this custom
+ # This section automatically populates a div with the "IncludeContentForValue" for this custom
# field if it's been defined
if ( $cf->IncludeContentForValue ) {
my $vid = $value->id;
$m->out( '<div class="object_cf_value_include" id="object_cf_value_'. $vid .'">' );
$m->print( loc("See also:") );
- $m->out( '<a href="'. $value->IncludeContentForValue .'">' );
- $m->print( $value->IncludeContentForValue );
+ $m->out( '<a href="'. $m->interp->apply_escapes($value->IncludeContentForValue, 'h') .'">' );
+ $m->out( $m->interp->apply_escapes($value->IncludeContentForValue, 'h') );
$m->out( qq{</a></div>\n} );
- $m->out( qq{<script><!--\nahah('} );
- $m->print( $value->IncludeContentForValue );
- $m->out( qq{', 'object_cf_value_$vid');\n--></script>\n} );
+ $m->out( qq{<script><!--\njQuery('#object_cf_value_$vid').load(} );
+ $m->out( $m->interp->apply_escapes($value->IncludeContentForValue, 'j') );
+ $m->out( qq{);\n--></script>\n} );
}
};
commit e47c6fbbb19790089134dee5af9c1e89bf88809b
Author: Thomas Sibley <trs at bestpractical.com>
Date: Tue Nov 15 16:23:19 2011 -0500
Prevent linking directly to CF values when the value is a data: URI
You can still create data: URIs in a linked CF with the value as part of
the URI, but the whole value can't be a data: URI itself.
This resolves part of CVE-2011-2083.
diff --git a/lib/RT/ObjectCustomFieldValue.pm b/lib/RT/ObjectCustomFieldValue.pm
index c6c7882..19e56e7 100644
--- a/lib/RT/ObjectCustomFieldValue.pm
+++ b/lib/RT/ObjectCustomFieldValue.pm
@@ -364,11 +364,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;
commit 8fdfef724fa75dda553679a6a30fe7d7cc60bd8b
Author: Thomas Sibley <trs at bestpractical.com>
Date: Wed Nov 16 17:07:47 2011 -0500
Escape width and wrap parameters when rendering a message box
This resolves part of CVE-2011-2083.
diff --git a/share/html/Elements/MessageBox b/share/html/Elements/MessageBox
index cd009db..9fb5d29 100755
--- a/share/html/Elements/MessageBox
+++ b/share/html/Elements/MessageBox
@@ -45,7 +45,7 @@
%# those contributions and any derivatives thereof.
%#
%# END BPS TAGGED BLOCK }}}
-<textarea autocomplete="off" class="messagebox" <% $cols |n %> rows="<% $Height %>" <% $wrap_type |n %> name="<% $Name %>" id="<% $Name %>">\
+<textarea autocomplete="off" class="messagebox" <% $width_attr %>="<% $Width %>" rows="<% $Height %>" <% $wrap_type |n %> name="<% $Name %>" id="<% $Name %>">\
% $m->comp('/Articles/Elements/IncludeArticle', %ARGS);
% $m->callback( %ARGS, SignatureRef => \$signature );
<% $Default || '' %><% $message %><% $signature %></textarea>
@@ -68,13 +68,16 @@ if ( $IncludeSignature and my $text = $session{'CurrentUser'}->UserObj->Signatur
# wrap="something" seems to really break IE + richtext
my $wrap_type = '';
if ( not RT->Config->Get('MessageBoxRichText', $session{'CurrentUser'}) ) {
- $wrap_type = qq(wrap="$Wrap");
+ $wrap_type = 'wrap="' . $m->interp->apply_escapes($Wrap, 'h') . '"';
}
-# If there's no cols specified, we want to set the width to 100%
-my $cols = 'style="width: 100%"';
-if ( defined $Width and length $Width ) {
- $cols = qq(cols="$Width");
+# If there's no cols specified, we want to set the width to 100% in CSS
+my $width_attr;
+if ($Width) {
+ $width_attr = 'cols';
+} else {
+ $width_attr = 'style';
+ $Width = 'width: 100%';
}
</%INIT>
commit 475780b6817d5a1c3de54bd524e3fc7426077460
Author: Thomas Sibley <trs at bestpractical.com>
Date: Wed Mar 28 18:33:56 2012 -0400
Escape NamePrefix to avoid XSS if it's passed into EditCustomField
This resolves part of CVE-2011-2083. Ticket #42936.
diff --git a/share/html/Elements/EditCustomField b/share/html/Elements/EditCustomField
index 6c5d7f5..32ea59d 100644
--- a/share/html/Elements/EditCustomField
+++ b/share/html/Elements/EditCustomField
@@ -85,7 +85,7 @@ if ($MaxValues == 1 && $Values) {
}
# The "Magic" hidden input causes RT to know that we were trying to edit the field, even if
# we don't see a value later, since browsers aren't compelled to submit empty form fields
-$m->out("\n".'<input type="hidden" class="hidden" name="'.$NamePrefix.$CustomField->Id.'-Values-Magic" value="1" />'."\n");
+$m->out("\n".'<input type="hidden" class="hidden" name="'.$m->interp->apply_escapes($NamePrefix, 'h').$CustomField->Id.'-Values-Magic" value="1" />'."\n");
my $EditComponent = "EditCustomField$Type";
$m->callback( %ARGS, CallbackName => 'EditComponentName', Name => \$EditComponent, CustomField => $CustomField, Object => $Object );
commit 2b5e6c9ff6cb1aeef306d3f83887099a8036ac37
Author: Thomas Sibley <trs at bestpractical.com>
Date: Thu Mar 29 13:47:42 2012 -0400
Close an XSS vector via BaseURL in collection lists
Most uses of CollectionList don't pass in a user-modifiable BaseURL, so
they're generally safe.
Resolves part of CVE-2011-2083. Ticket #58595.
diff --git a/share/html/Elements/CollectionAsTable/Header b/share/html/Elements/CollectionAsTable/Header
index 0a4e2f7..5084f82 100644
--- a/share/html/Elements/CollectionAsTable/Header
+++ b/share/html/Elements/CollectionAsTable/Header
@@ -129,11 +129,11 @@ foreach my $col ( @Format ) {
if $OrderBy[0] && ($OrderBy[0] eq $attr or "$attr|$OrderBy[0]" =~ /^(Created|id)\|(Created|id)$/);
$m->out(
- '<a href="' . $BaseURL
+ '<a href="' . $m->interp->apply_escapes($BaseURL
. $m->comp( '/Elements/QueryString',
%$generic_query_args,
OrderBy => $attr, Order => $new_order
- )
+ ), 'h')
. '">'. loc($title) .'</a>'
);
}
diff --git a/share/html/Elements/CollectionListPaging b/share/html/Elements/CollectionListPaging
index 7be9ea6..89cf0fa 100644
--- a/share/html/Elements/CollectionListPaging
+++ b/share/html/Elements/CollectionListPaging
@@ -55,22 +55,24 @@ $URLParams => undef
</%ARGS>
<%INIT>
+$BaseURL = $m->interp->apply_escapes($BaseURL, 'h');
+
$m->out(qq{<div class="paging">});
if ($Pages == 1) {
$m->out(loc('Page 1 of 1'));
}
else{
$m->out(loc('Page') . ' ');
-my $prev = $m->comp(
+my $prev = $m->interp->apply_escapes($m->comp(
'/Elements/QueryString',
%$URLParams,
Page => ( $CurrentPage - 1 )
- );
-my $next = $m->comp(
+ ), 'h');
+my $next = $m->interp->apply_escapes($m->comp(
'/Elements/QueryString',
%$URLParams,
Page => ( $CurrentPage + 1 )
- );
+ ), 'h');
my %show;
$show{1} = 1;
$show{$_} = 1 for (($CurrentPage - 2)..($CurrentPage + 2));
@@ -81,7 +83,7 @@ for my $number ( 1 .. $Pages ) {
if ( $show{$number} ) {
$dots = undef;
my $qs =
- $m->comp( '/Elements/QueryString', %$URLParams, Page => $number );
+ $m->interp->apply_escapes($m->comp( '/Elements/QueryString', %$URLParams, Page => $number ), 'h');
$m->out(qq{<span class="pagenum">});
if ( $number == $CurrentPage ) {
$m->out(qq{<span class="currentpage">$number</span> });
commit 93cb7cb1d09352627a7060e50821aca1ea5924aa
Merge: e0ac46a 2b5e6c9
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Wed Apr 11 14:49:59 2012 -0400
Merge branch 'security/4.0/xss' into security/4.0-trunk
Conflicts:
lib/RT/Interface/Web.pm
share/html/Elements/EditCustomFieldAutocomplete
share/html/Ticket/Elements/ClickToShowHistory
diff --cc lib/RT/Interface/Web.pm
index 80e4075,04e4b87..7274c6b
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@@ -147,17 -146,25 +147,36 @@@ sub EscapeURI
$$ref =~ s/([^a-zA-Z0-9_.!~*'()-])/uc sprintf("%%%02X", ord($1))/eg;
}
+=head2 EncodeJSON SCALAR
+
+Encodes the SCALAR to JSON and returns a JSON string. SCALAR may be a simple
+value or a reference.
+
+=cut
+
+sub EncodeJSON {
+ JSON::to_json(shift, { utf8 => 1, allow_nonref => 1 });
+}
+
+ 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))
+ . "'";
+ }
+
=head2 WebCanonicalizeInfo();
Different web servers set different environmental varibles. This
diff --cc share/html/Elements/EditCustomFieldAutocomplete
index aaf5517,7b45e2f..1386eff
--- a/share/html/Elements/EditCustomFieldAutocomplete
+++ b/share/html/Elements/EditCustomFieldAutocomplete
@@@ -73,10 -73,10 +73,10 @@@ jQuery('#'+id).autocomplete(
% } else {
<input type="text" id="<% $name %>-Value" name="<% $name %>-Value" class="CF-<%$CustomField->id%>-Edit" value="<% $Default || '' %>"/>
<script type="text/javascript">
- var id = '<% $name . '-Value' %>';
+ var id = <% "$name-Value" |n,j%>;
id = id.replace(/:/g,'\\:');
jQuery('#'+id).autocomplete( {
- source: "<%RT->Config->Get('WebPath')%>/Helpers/Autocomplete/CustomFieldValues?<% $name . '-Value' %>"
- source: <%RT->Config->Get('WebPath')|n,j%>+"/Helpers/Autocomplete/CustomFieldValues?"+<% "$name-Value" |n,u,j%>,
++ source: <%RT->Config->Get('WebPath')|n,j%>+"/Helpers/Autocomplete/CustomFieldValues?"+<% "$name-Value" |n,u,j%>
}
);
% }
diff --cc share/html/Ticket/Elements/ClickToShowHistory
index 4461b9a,cee2e60..5a9a477
--- a/share/html/Ticket/Elements/ClickToShowHistory
+++ b/share/html/Ticket/Elements/ClickToShowHistory
@@@ -47,7 -47,7 +47,7 @@@
%# END BPS TAGGED BLOCK }}}
<div id="deferred_ticket_history">
<& /Widgets/TitleBoxStart, title => 'History' &>
- <a href="<% $display %>" onclick="jQuery('#deferred_ticket_history').text('<% loc('Loading...') %>').load('<% $url |n %>'); return false;" ><% loc('Show ticket history') %></a>
- <a href="<% $url %>" onclick="jQuery('#deferred_ticket_history').text(<% loc('Loading...') |n,j%>).load(<% $url |n,j %>); return false;" ><% loc('Show ticket history') %></a>
++ <a href="<% $display %>" onclick="jQuery('#deferred_ticket_history').text(<% loc('Loading...') |n,j%>).load(<% $url |n,j %>); return false;" ><% loc('Show ticket history') %></a>
<& /Widgets/TitleBoxEnd &>
</div>
<%ARGS>
commit 41a266405b9809d1e9dc0fc5335cf7683460b813
Merge: 93cb7cb 928e123
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Wed Apr 11 14:50:13 2012 -0400
Merge branch 'security/4.0/clickable-xss-links' into security/4.0-trunk
commit feec1c6e775de48a0c95c359ea8cc70bbf1d5538
Merge: 41a2664 d1655ad
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Wed Apr 11 14:50:19 2012 -0400
Merge branch 'security/4.0/mason-runtime-errors' into security/4.0-trunk
commit 9aa0957f42d354df6d1848c7736647ef88c9e29e
Merge: feec1c6 29f7442
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Wed Apr 11 14:50:24 2012 -0400
Merge branch 'security/4.0/scrub-class-id' into security/4.0-trunk
commit a23c3260aea61415135b35eb9efba3b52ee7187a
Merge: 9aa0957 b7393fb
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Wed Apr 11 14:50:28 2012 -0400
Merge branch 'security/4.0/articles-escaping' into security/4.0-trunk
commit 1d7cc2220480f5d7e9f37994f01c1958aac960fb
Merge: a23c326 08b7989f
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Wed Apr 11 14:50:41 2012 -0400
Merge branch 'security/4.0/stricter-scrips-templates-acls' into security/4.0-trunk
commit c68172b9a7b8e045215e70f1490145164cd00ab6
Merge: 1d7cc22 87aa1d4
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Wed Apr 11 14:50:49 2012 -0400
Merge branch 'security/4.0/selfservice' into security/4.0-trunk
commit 22fa8d088839c8d66c7d6311e4031aa62d7008f0
Merge: c68172b 0d10462
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Wed Apr 11 14:50:58 2012 -0400
Merge branch 'security/4.0/shredder-dumps' into security/4.0-trunk
commit 5242c76b43961555de802c7de26a605df34c02d0
Merge: 22fa8d0 c29107c
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Wed Apr 11 14:51:15 2012 -0400
Merge branch 'security/4.0/attachments' into security/4.0-trunk
Conflicts:
lib/RT/Action/SendEmail.pm
diff --cc lib/RT/Action/SendEmail.pm
index e2aa00b,0105373..94686b8
--- a/lib/RT/Action/SendEmail.pm
+++ b/lib/RT/Action/SendEmail.pm
@@@ -408,16 -408,15 +408,20 @@@ 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;
+
+ # ->attach expects just the disposition type; extract it if we have the header
+ my $disp = ($attach->GetHeader('Content-Disposition') || '')
+ =~ /^\s*(inline|attachment)/i ? $1 : undef;
+
$MIMEObj->attach(
- Type => $attach->ContentType,
- Charset => $attach->OriginalEncoding,
- Data => $attach->OriginalContent,
- Filename => $self->MIMEEncodeString( $attach->Filename ),
+ Type => $attach->ContentType,
+ Charset => $attach->OriginalEncoding,
+ Data => $attach->OriginalContent,
+ Disposition => $disp, # a false value defaults to inline in MIME::Entity
+ Filename => $self->MIMEEncodeString( $attach->Filename ),
'RT-Attachment:' => $self->TicketObj->Id . "/"
. $self->TransactionObj->Id . "/"
. $attach->id,
commit c44e395e91292392fbd8d36821220b6f71b40474
Merge: 5242c76 dbd7871
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Wed Apr 11 14:51:25 2012 -0400
Merge branch 'security/4.0/cached-set-cookie' into security/4.0-trunk
commit 855906aa2850c6277688536d7df532e25529efe6
Merge: c44e395 59d2fb3
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Wed Apr 11 14:51:35 2012 -0400
Merge branch 'security/4.0/transaction-leak' into security/4.0-trunk
commit 0ca6a53efd94155c1fb7a2b09859156f7a05edf3
Merge: 855906a 6ef92fe
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Wed Apr 11 14:57:04 2012 -0400
Merge branch 'security/4.0/csrf-referer' into security/4.0-trunk
diff --cc share/html/Elements/CSRF
index 50f3b77,ff658dd..4893c12
mode 100755,100644..100644
--- a/share/html/Elements/CSRF
+++ b/share/html/Elements/CSRF
@@@ -45,46 -45,30 +45,30 @@@
%# those contributions and any derivatives thereof.
%#
%# END BPS TAGGED BLOCK }}}
- % $m->callback( %ARGS, error => $error );
-
- % unless ($SuppressHeader) {
- <& /Elements/Header, Code => $Code, Why => $Why, Title => $Title &>
+ <& /Elements/Header, Title => loc('Possible cross-site request forgery') &>
<& /Elements/Tabs &>
- % }
- <div class="error">
- <%$Why%>
- <br />
- <%$Details%>
- </div>
+ <h1><&|/l&>Possible cross-site request forgery</&></h1>
- <%cleanup>
- $m->comp('/Elements/Footer');
- $m->abort();
- </%cleanup>
+ % my $strong_start = "<strong>";
+ % my $strong_end = "</strong>";
-<p><&|/l, $strong_start, $strong_end, $Reason &>RT has detected a possible [_1]cross-site request forgery[_2] for this request, because [_3]. This is possibly caused by a malicious attacker trying to perform actions against RT on your behalf. If you did not initiate this request, then you should alert your security team.</&></p>
++<p><&|/l_unsafe, $strong_start, $strong_end, $Reason &>RT has detected a possible [_1]cross-site request forgery[_2] for this request, because [_3]. This is possibly caused by a malicious attacker trying to perform actions against RT on your behalf. If you did not initiate this request, then you should alert your security team.</&></p>
- <%args>
- $Code => undef
- $Details => ''
- $Title => loc("RT Error")
- $Why => loc("the calling component did not specify why"),
- $SuppressHeader => 0,
- </%args>
+ % my $start = qq|<strong><a href="$url_with_token">|;
+ % my $end = qq|</a></strong>|;
-<p><&|/l, $escaped_path, $start, $end &>If you really intended to visit [_1], then [_2]click here to resume your request[_3].</&></p>
++<p><&|/l_unsafe, $escaped_path, $start, $end &>If you really intended to visit [_1], then [_2]click here to resume your request[_3].</&></p>
+ <& /Elements/Footer, %ARGS &>
+ % $m->abort;
+ <%ARGS>
+ $OriginalURL => ''
+ $Reason => ''
+ $Token => ''
+ </%ARGS>
<%INIT>
- my $error = "WebRT: $Why";
- $error .= " ($Details)" if defined $Details && length $Details;
-
- # TODO: Log::Dispatch isn't UTF-8 safe. Autrijus needs to talk to dave rolsky about getting this fixed
- use Encode ();
- Encode::_utf8_off($error);
-
- $RT::Logger->error($error);
+ my $escaped_path = $m->interp->apply_escapes($OriginalURL, 'h');
+ $escaped_path = "<tt>$escaped_path</tt>";
- if ( defined $session{'SessionType'} && $session{'SessionType'} eq 'REST' ) {
- $r->content_type('text/plain');
- $m->out( "Error: " . $Why . "\n" );
- $m->out( $Details . "\n" ) if defined $Details && length $Details;
- $m->abort();
- }
+ my $url_with_token = URI->new($OriginalURL);
+ $url_with_token->query_form([CSRF_Token => $Token]);
</%INIT>
commit eb74e9568157a1027aa9c4d71fb9b38a4e3323e8
Merge: 0ca6a53 04a9551
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Wed Apr 11 14:57:31 2012 -0400
Merge branch 'security/4.0/arbitrary-methods' into security/4.0-trunk
commit bac33a25630ef70be3efe3635789b08f48228093
Merge: eb74e95 303affe
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Wed Apr 11 14:57:36 2012 -0400
Merge branch 'security/4.0/disallow-execute-code' into security/4.0-trunk
commit 06ea1ab348159999e5563ac72a4deecc4e203c37
Merge: bac33a2 74ab1ea
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Wed Apr 11 14:57:37 2012 -0400
Merge branch 'security/4.0/verp-code-execution' into security/4.0-trunk
commit a9bd59f450af6a3540d114f4fc9c9b148e9d5548
Merge: 06ea1ab 5f265b6
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Wed Apr 11 14:57:38 2012 -0400
Merge branch 'security/4.0/private-components' into security/4.0-trunk
commit 22bbf1944adcef38a497236ac5d691280d2b91cd
Merge: a9bd59f a3c6991
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Wed Apr 11 14:57:40 2012 -0400
Merge branch 'security/4.0/installmode' into security/4.0-trunk
commit b88578beb8179583acb6ee310ba0e757bef44614
Merge: 22bbf19 4881ae8
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Wed Apr 11 14:57:41 2012 -0400
Merge branch 'security/4.0/paging-injection' into security/4.0-trunk
commit 09ec4163b57c60cdb42c610a77ce431fab7d787f
Merge: b88578b 55cb6f4
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Wed Apr 11 14:57:42 2012 -0400
Merge branch 'security/4.0/graphviz-escaping' into security/4.0-trunk
commit dbb8542375f98daa79cf12151589d6ad0158fddf
Merge: 09ec416 08754c0
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Wed Apr 11 14:57:49 2012 -0400
Merge branch 'security/4.0/custom-field-values' into security/4.0-trunk
Conflicts:
share/html/Elements/EditCustomFieldAutocomplete
diff --cc share/html/Elements/EditCustomFieldAutocomplete
index 1386eff,0cef26f..911e607
--- a/share/html/Elements/EditCustomFieldAutocomplete
+++ b/share/html/Elements/EditCustomFieldAutocomplete
@@@ -49,10 -49,10 +49,10 @@@
<textarea cols="<% $Cols %>" rows="<% $Rows %>" name="<% $name %>-Values" id="<% $name %>-Values" class="CF-<%$CustomField->id%>-Edit"><% $Default || '' %></textarea>
<script type="text/javascript">
-var id = '<% $name . '-Values' %>';
+var id = <% "$name-Values" |n,j%>;
id = id.replace(/:/g,'\\:');
jQuery('#'+id).autocomplete( {
- source: <%RT->Config->Get('WebPath') |n,j%>+"/Helpers/Autocomplete/CustomFieldValues?"+<% "$name-Values" |n,u,j%>,
- source: "<%RT->Config->Get('WebPath')%>/Helpers/Autocomplete/CustomFieldValues?<% $Context |n %><% $name . '-Values' %>",
++ source: <%RT->Config->Get('WebPath') |n,j%>+"/Helpers/Autocomplete/CustomFieldValues?"+<% $Context |n,j %>+<% "$name-Values" |n,u,j%>,
focus: function () {
// prevent value inserted on focus
return false;
@@@ -73,10 -73,10 +73,10 @@@
% } else {
<input type="text" id="<% $name %>-Value" name="<% $name %>-Value" class="CF-<%$CustomField->id%>-Edit" value="<% $Default || '' %>"/>
<script type="text/javascript">
-var id = '<% $name . '-Value' %>';
+var id = <% "$name-Value" |n,j%>;
id = id.replace(/:/g,'\\:');
jQuery('#'+id).autocomplete( {
- source: <%RT->Config->Get('WebPath')|n,j%>+"/Helpers/Autocomplete/CustomFieldValues?"+<% "$name-Value" |n,u,j%>
- source: "<%RT->Config->Get('WebPath')%>/Helpers/Autocomplete/CustomFieldValues?<% $Context |n %><% $name . '-Value' %>",
++ source: <%RT->Config->Get('WebPath')|n,j%>+"/Helpers/Autocomplete/CustomFieldValues?"+<% $Context |n,j %>+<% "$name-Value" |n,u,j%>
}
);
% }
commit 4ff6192e94b193def986b970f7c219b80cd8aa9b
Merge: dbb8542 fbef48d
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Wed Apr 11 14:57:53 2012 -0400
Merge branch 'security/4.0/disabled-group-members' into security/4.0-trunk
commit 10a3bb4c825247aeb1ffab10bb1bb0f4e40ead6c
Merge: 4ff6192 4faebb1
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Wed Apr 11 14:57:55 2012 -0400
Merge branch 'security/4.0/secure-portal-iframe' into security/4.0-trunk
commit 85142adb3b62e4d90454d28933b04ebade7b206f
Merge: 10a3bb4 4209699
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Wed Apr 11 14:57:56 2012 -0400
Merge branch 'security/4.0/infrastructure' into security/4.0-trunk
commit 078257dc4b9da5f5575c257fd1a5f0cee044a200
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Wed Apr 11 22:10:07 2012 -0400
Update test to account for new parameters
diff --git a/t/web/case-sensitivity.t b/t/web/case-sensitivity.t
index 276b761..f984bf3 100644
--- a/t/web/case-sensitivity.t
+++ b/t/web/case-sensitivity.t
@@ -75,7 +75,7 @@ my $cf;
# test custom field values auto completer
{
- $m->get_ok('/Helpers/Autocomplete/CustomFieldValues?term=eNo&Object---CustomField-'. $cf->id .'-Value');
+ $m->get_ok('/Helpers/Autocomplete/CustomFieldValues?term=eNo&Object---CustomField-'. $cf->id .'-Value&ContextId=1&ContextType=RT::Queue');
require JSON;
is_deeply(
JSON::from_json( $m->content ),
commit 4c486f95227079fcb367f1a3882feeae33edf7a1
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Fri Apr 13 00:03:30 2012 -0400
Remove an incorrect Disabled limit
diff --git a/lib/RT/Users.pm b/lib/RT/Users.pm
index 6e1cfe7..787ac10 100644
--- a/lib/RT/Users.pm
+++ b/lib/RT/Users.pm
@@ -292,11 +292,6 @@ sub _JoinGroups
ALIAS2 => $group_members,
FIELD2 => 'GroupId'
);
- $self->Limit(
- ALIAS => $groups,
- FIELD => 'Disabled',
- VALUE => 0,
- );
return $groups;
}
commit e681fa720ebe8a6d6949dae6f72ccce1f06f9397
Author: Kevin Falcone <falcone at bestpractical.com>
Date: Fri Apr 13 16:45:33 2012 -0400
Tell users and admins what Referrer we wanted
This introduces a normalizing method we could use elsewhere in the Web
code, as well as uses that code to hide the localhost->127.0.0.1
transformations.
It also adds to the error string so that you know what RT was expecting.
"RT's configured hostname" is the best we could do without explicitly
stating WebBaseURL in a user facing error message.
diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index 5f22c10..172add6 100644
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -1208,13 +1208,28 @@ sub IsCompCSRFWhitelisted {
}
sub IsRefererCSRFWhitelisted {
- my $referer = shift;
+ my $referer = _NormalizeHost(shift);
+ my $config = _NormalizeHost(RT->Config->Get('WebBaseURL'));
- my $site = URI->new(RT->Config->Get('WebBaseURL'));
- $site->host('127.0.0.1') if $site->host eq 'localhost';
- return 1 if $referer->host_port eq $site->host_port;
+ return (1,$referer,$config) if $referer->host_port eq $config->host_port;
+
+ return (0,$referer,$config);
+}
+
+=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;
- return 0;
}
sub IsPossibleCSRF {
@@ -1253,13 +1268,12 @@ EOT
"your browser did not supply a Referrer header", # loc
) if !$ENV{HTTP_REFERER};
- my $referer = URI->new($ENV{HTTP_REFERER});
- $referer->host('127.0.0.1') if $referer->host eq 'localhost';
- return 0 if IsRefererCSRFWhitelisted($referer);
+ my ($whitelisted, $browser, $config) = IsRefererCSRFWhitelisted($ENV{HTTP_REFERER});
+ return 0 if $whitelisted;
return (1,
- "the Referrer header supplied by your browser ([_1]) is not allowed", # loc
- $referer->host_port);
+ "the Referrer header supplied by your browser ([_1]) is not allowed by RT's configured hostname ([_2])", # loc
+ $browser->host_port, $config->host_port);
}
sub ExpandCSRFToken {
commit 4786981af61f69a5734f7ac38b394aaab82771a4
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Wed Apr 18 16:09:31 2012 -0400
Use l_unsafe, as $path_tag contains an unescaped <span>
diff --git a/share/html/Admin/Tools/Shredder/Elements/Error/NoStorage b/share/html/Admin/Tools/Shredder/Elements/Error/NoStorage
index ce93411..ae3b96e 100644
--- a/share/html/Admin/Tools/Shredder/Elements/Error/NoStorage
+++ b/share/html/Admin/Tools/Shredder/Elements/Error/NoStorage
@@ -52,5 +52,5 @@ $Path => ''
<& /Elements/Tabs &>
<div class="error">
% my $path_tag = q{<span class="file-path">} . $m->interp->apply_escapes($Path, 'h') . q{</span>};
-<&|/l, $path_tag &>Shredder needs a directory to write dumps to. Please ensure that the directory [_1] exists and that it is writable by your web server.</&>
+<&|/l_unsafe, $path_tag &>Shredder needs a directory to write dumps to. Please ensure that the directory [_1] exists and that it is writable by your web server.</&>
</div>
commit 21da57aba3248b21240954274bcf5d9a47c92b49
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Wed Apr 18 16:13:53 2012 -0400
Safety-checking on classes loaded with `eval "require $class"`
While these close an arbitrary execution of code vulnerability, it
required SuperUser privileges to exploit. As SuperUsers already have
the ability to run arbitrary code using Scrips, this vulnerability was
primarily one of CSRF, which is closed by CSRF protection. Regardless,
validate the package names before they are inserted into the string
eval.
diff --git a/lib/RT/Shredder.pm b/lib/RT/Shredder.pm
index 10d3536..40c73b3 100644
--- a/lib/RT/Shredder.pm
+++ b/lib/RT/Shredder.pm
@@ -351,6 +351,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/lib/RT/Shredder/Plugin.pm b/lib/RT/Shredder/Plugin.pm
index e70d207..ad9af6a 100644
--- a/lib/RT/Shredder/Plugin.pm
+++ b/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";
commit fa9c4b4b218ea231c048312a3ca0be76b3231a1e
Merge: 080eb2a 21da57a
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Wed Apr 25 20:06:49 2012 -0400
Merge branch 'security/4.0-trunk' into 4.0-trunk
Conflicts:
lib/RT/Interface/Web.pm
diff --cc lib/RT/Interface/Web.pm
index 8e89ce8,172add6..0514d62
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@@ -2831,6 -3062,40 +3062,42 @@@ following
=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
++ H6 DIV UL OL LI DL DT DD PRE BLOCKQUOTE BDO
+ );
+
+ 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,
++ dir => qr/^(rtl|ltr)$/i,
++ lang => qr/^\w+(-\w+)?$/,
+ );
+
+ our %SCRUBBER_RULES = ();
+
sub _NewScrubber {
require HTML::Scrubber;
my $scrubber = HTML::Scrubber->new();
commit 5c96ad518540645f3daa76111ebb07109c75c0ab
Author: Jim Brandt <jbrandt at bestpractical.com>
Date: Mon Apr 30 16:18:18 2012 -0400
Add WebPath to link created on CSRF interstitial page.
diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index 0514d62..0f70ceb 100644
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -1340,7 +1340,7 @@ sub MaybeShowInterstitialCSRFPage {
$HTML::Mason::Commands::m->comp(
'/Elements/CSRF',
- OriginalURL => $HTML::Mason::Commands::r->path_info,
+ OriginalURL => RT->Config->Get('WebPath') . $HTML::Mason::Commands::r->path_info,
Reason => HTML::Mason::Commands::loc( $msg, @loc ),
Token => $token,
);
commit 6dc6a0bb9f043b5698349a0d5c946fe58029a36c
Author: Kevin Falcone <falcone at bestpractical.com>
Date: Tue May 1 14:18:43 2012 -0400
Fix a simple typo
diff --git a/etc/RT_Config.pm.in b/etc/RT_Config.pm.in
index c560c5e..765dc51 100755
--- a/etc/RT_Config.pm.in
+++ b/etc/RT_Config.pm.in
@@ -1774,7 +1774,7 @@ Set($RestrictReferrer, 1);
If set to a false value, RT will allow the user to log in from any link
or request, merely by passing in C<user> and C<pass> parameters; setting
it to a true value forces all logins to come from the login box, so the
-user us aware that they are being logged in. The default is off, for
+user is aware that they are being logged in. The default is off, for
backwards compatability.
=cut
commit 6d390c32367a820a413defce2677cf8a3b3a1ad1
Author: Kevin Falcone <falcone at bestpractical.com>
Date: Tue May 1 21:46:35 2012 -0400
Switch to our so that extensions can whitelist components
Also rename the variable because you're actually whitelisting a
component, especially something like a dhandler which won't be in the
URL.
diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index 172add6..c340f88 100644
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -1161,7 +1161,7 @@ sub ComponentRoots {
return @roots;
}
-my %is_whitelisted_path = (
+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.
@@ -1172,7 +1172,7 @@ sub IsCompCSRFWhitelisted {
my $comp = shift;
my $ARGS = shift;
- return 1 if $is_whitelisted_path{$comp};
+ return 1 if $is_whitelisted_component{$comp};
my %args = %{ $ARGS };
commit d9ab0d48e09a24ecad965e021eab31366ae6b860
Author: Kevin Falcone <falcone at bestpractical.com>
Date: Tue May 1 22:41:00 2012 -0400
Add a new ReferrerWhitelist config option
This is a list of hostname:port that RT will accept HTTP_REFERER for.
This is helpful if your RT has two hostnames or if you need to have auth
from an external service that redirects back into RT.
diff --git a/etc/RT_Config.pm.in b/etc/RT_Config.pm.in
index c560c5e..4e12bba 100755
--- a/etc/RT_Config.pm.in
+++ b/etc/RT_Config.pm.in
@@ -1781,6 +1781,19 @@ backwards compatability.
Set($RestrictLoginReferrer, 0);
+=item C<$ReferrerWhitelist>
+
+This is a list of hostname:port combinations that RT will treat as being
+part of RT's domain. This is particularly useful if you access RT as
+multiple hostnames or have an external auth system that needs to
+redirect back to RT once authentication is complete.
+
+ Set(@ReferrerWhitelist, qw(www.example.com:443 www3.example.com:80));
+
+=cut
+
+Set(@ReferrerWhitelist, qw());
+
=back
diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index c340f88..1919b12 100644
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -1209,11 +1209,16 @@ sub IsCompCSRFWhitelisted {
sub IsRefererCSRFWhitelisted {
my $referer = _NormalizeHost(shift);
- my $config = _NormalizeHost(RT->Config->Get('WebBaseURL'));
+ my $base_url = _NormalizeHost(RT->Config->Get('WebBaseURL'));
+ $base_url = $base_url->host_port;
- return (1,$referer,$config) if $referer->host_port eq $config->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,$config);
+ return (0,$referer,$configs);
}
=head3 _NormalizeHost
@@ -1268,12 +1273,14 @@ EOT
"your browser did not supply a Referrer header", # loc
) if !$ENV{HTTP_REFERER};
- my ($whitelisted, $browser, $config) = IsRefererCSRFWhitelisted($ENV{HTTP_REFERER});
+ my ($whitelisted, $browser, $configs) = IsRefererCSRFWhitelisted($ENV{HTTP_REFERER});
return 0 if $whitelisted;
return (1,
- "the Referrer header supplied by your browser ([_1]) is not allowed by RT's configured hostname ([_2])", # loc
- $browser->host_port, $config->host_port);
+ "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) );
}
sub ExpandCSRFToken {
commit 8680717f01a26e656b74fd9ca1c9bfd1720e5519
Author: Kevin Falcone <falcone at bestpractical.com>
Date: Tue May 1 23:05:40 2012 -0400
Document how to pull from the error into the config
diff --git a/etc/RT_Config.pm.in b/etc/RT_Config.pm.in
index 4e12bba..7fe9c76 100755
--- a/etc/RT_Config.pm.in
+++ b/etc/RT_Config.pm.in
@@ -1790,6 +1790,10 @@ redirect back to RT once authentication is complete.
Set(@ReferrerWhitelist, qw(www.example.com:443 www3.example.com:80));
+If the "RT has detected a possible cross-site request forgery" error is triggered
+by a host:port sent by your browser that you believe should be valid, you can copy
+the host:port from the error message into this list.
+
=cut
Set(@ReferrerWhitelist, qw());
commit 7bc96758e8c01b067de13aa5d3a06509ebcab802
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Thu May 3 23:03:29 2012 -0400
Allow the homepage refresh argument as an idempotent query parameter
diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index 172add6..72cb502 100644
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -1200,6 +1200,11 @@ sub IsCompCSRFWhitelisted {
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;
commit f9390c4962591a77046c13070019aa960e071c6d
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Thu May 3 23:16:23 2012 -0400
Abstract out creation of request tokens which bypass CSRF
diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index 72cb502..6e2cee7 100644
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -1309,21 +1309,9 @@ sub ExpandCSRFToken {
return 1;
}
-sub MaybeShowInterstitialCSRFPage {
+sub StoreRequestToken {
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 = Digest::MD5::md5_hex(time . {} . $$ . rand(1024));
my $user = $HTML::Mason::Commands::session{'CurrentUser'}->UserObj;
my $data = {
@@ -1342,7 +1330,25 @@ sub MaybeShowInterstitialCSRFPage {
$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->path_info,
commit 906e9a34c52d890e5d69533d9fd35c9547f0cb43
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Thu May 3 23:38:32 2012 -0400
Rename LogoutURL to the more general-use RefreshURL
diff --git a/share/html/Elements/Header b/share/html/Elements/Header
index 4b0c2b8..4a6ac26 100755
--- a/share/html/Elements/Header
+++ b/share/html/Elements/Header
@@ -57,7 +57,7 @@
<& /Elements/Framekiller &>
% if ($Refresh && $Refresh =~ /^(\d+)/ && $1 > 0) {
-% my $URL = $m->notes->{LogoutURL}; $URL = $URL ? ";URL=$URL" : "";
+% my $URL = $m->notes->{RefreshURL}; $URL = $URL ? ";URL=$URL" : "";
<meta http-equiv="refresh" content="<% "$1$URL" %>" />
% }
diff --git a/share/html/NoAuth/Logout.html b/share/html/NoAuth/Logout.html
index b8e119a..20024cc 100755
--- a/share/html/NoAuth/Logout.html
+++ b/share/html/NoAuth/Logout.html
@@ -81,5 +81,5 @@ if (keys %session) {
}
$m->callback( %ARGS, CallbackName => 'AfterSessionDelete' );
-$m->notes->{LogoutURL} = $URL;
+$m->notes->{RefreshURL} = $URL;
</%INIT>
commit 17bc0c17ab2523c7c73284ac8806954ad9ee573f
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Fri May 4 15:34:39 2012 -0400
Add a global argument which contains the decoded $m->request_args
Multiple locations in the code use $m->request_args to obtain
information about the query parameters that were specified in the URL;
however, the values recovered from this call are not utf8-decoded, which
can lead to corrupted data. Additionally, existing code may depend on
$m->request_args being encoded, which prevents merely altering the data
prior to its entry into Mason.
Provide a global variable, $DECODED_ARGS, which provides the correct,
decoded, query parameters.
diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index 172add6..5f59fd1 100644
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -253,6 +253,7 @@ sub HandleRequest {
ValidateWebConfig();
DecodeARGS($ARGS);
+ local $HTML::Mason::Commands::DECODED_ARGS = $ARGS;
PreprocessTimeUpdates($ARGS);
InitializeMenu();
diff --git a/lib/RT/Interface/Web/Handler.pm b/lib/RT/Interface/Web/Handler.pm
index f96f66e..a740167 100644
--- a/lib/RT/Interface/Web/Handler.pm
+++ b/lib/RT/Interface/Web/Handler.pm
@@ -69,7 +69,7 @@ sub DefaultHandlerArgs { (
],
default_escape_flags => 'h',
data_dir => "$RT::MasonDataDir",
- allow_globals => [qw(%session)],
+ allow_globals => [qw(%session $DECODED_ARGS)],
# Turn off static source if we're in developer mode.
static_source => (RT->Config->Get('DevelMode') ? '0' : '1'),
use_object_files => (RT->Config->Get('DevelMode') ? '0' : '1'),
commit 33840d670d3c801863a86446d1291880708a74ea
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Fri May 4 15:35:03 2012 -0400
Override $DECODED_ARGS with the (decoded) arguments from the CSRF token
The menuing code examines $m->request_args to determine some menu state.
Unfortunately, when returning from a CSRF interstitial the args provided
to the component have been inflated, but $m->request_args has not been,
and will only be observed to have one argument, CSRF_Token. While one
could, during CSRF argument inflation, replace $m->request_args by
reaching inside the object, this is not only naughty, but incorrect: the
query parameters stored in the CSRF token are already-decoded
parameters, while $m->request_args is expected to contain encoded
parameters.
The newly-introduced $DECODED_ARGS provides a centralized location which
is expected to contain decoded parameters. Replace calls to
$m->request_args with $DECODED_ARGS, and ensure that the latter is
updated when returning from a CSRF interstitial.
diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index 5f59fd1..cfcf000 100644
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -1291,6 +1291,7 @@ sub ExpandCSRFToken {
return unless $user->ValidateAuthString( $data->{auth}, $token );
%{$ARGS} = %{$data->{args}};
+ $HTML::Mason::Commands::DECODED_ARGS = $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
diff --git a/share/html/Elements/Tabs b/share/html/Elements/Tabs
index ccfbd4c..95cc21a 100755
--- a/share/html/Elements/Tabs
+++ b/share/html/Elements/Tabs
@@ -245,7 +245,7 @@ my $build_admin_menu = sub {
my $section;
if ( $request_path =~ m|^/Admin/$type/?(?:index.html)?$|
|| ( $request_path =~ m|^/Admin/$type/(?:Modify.html)$|
- && $m->request_args->{'Create'} )
+ && $DECODED_ARGS->{'Create'} )
)
{
$section = $tabs;
@@ -260,11 +260,11 @@ my $build_admin_menu = sub {
}
if ( $request_path =~ m{^/Admin/Queues} ) {
- if ( $m->request_args->{'id'} && $m->request_args->{'id'} =~ /^\d+$/
+ if ( $DECODED_ARGS->{'id'} && $DECODED_ARGS->{'id'} =~ /^\d+$/
||
- $m->request_args->{'Queue'} && $m->request_args->{'Queue'} =~ /^\d+$/
+ $DECODED_ARGS->{'Queue'} && $DECODED_ARGS->{'Queue'} =~ /^\d+$/
) {
- my $id = $m->request_args->{'Queue'} || $m->request_args->{'id'};
+ my $id = $DECODED_ARGS->{'Queue'} || $DECODED_ARGS->{'id'};
my $queue_obj = RT::Queue->new( $session{'CurrentUser'} );
$queue_obj->Load($id);
@@ -294,8 +294,8 @@ my $build_admin_menu = sub {
}
}
if ( $request_path =~ m{^/Admin/Users} ) {
- if ( $m->request_args->{'id'} && $m->request_args->{'id'} =~ /^\d+$/ ) {
- my $id = $m->request_args->{'id'};
+ if ( $DECODED_ARGS->{'id'} && $DECODED_ARGS->{'id'} =~ /^\d+$/ ) {
+ my $id = $DECODED_ARGS->{'id'};
my $obj = RT::User->new( $session{'CurrentUser'} );
$obj->Load($id);
@@ -312,8 +312,8 @@ my $build_admin_menu = sub {
}
if ( $request_path =~ m{^/Admin/Groups} ) {
- if ( $m->request_args->{'id'} && $m->request_args->{'id'} =~ /^\d+$/ ) {
- my $id = $m->request_args->{'id'};
+ if ( $DECODED_ARGS->{'id'} && $DECODED_ARGS->{'id'} =~ /^\d+$/ ) {
+ my $id = $DECODED_ARGS->{'id'};
my $obj = RT::Group->new( $session{'CurrentUser'} );
$obj->Load($id);
@@ -327,8 +327,8 @@ my $build_admin_menu = sub {
}
if ( $request_path =~ m{^/Admin/CustomFields/} ) {
- if ( $m->request_args->{'id'} && $m->request_args->{'id'} =~ /^\d+$/ ) {
- my $id = $m->request_args->{'id'};
+ if ( $DECODED_ARGS->{'id'} && $DECODED_ARGS->{'id'} =~ /^\d+$/ ) {
+ my $id = $DECODED_ARGS->{'id'};
my $obj = RT::CustomField->new( $session{'CurrentUser'} );
$obj->Load($id);
@@ -353,7 +353,7 @@ my $build_admin_menu = sub {
if ( $request_path =~ m{^/Admin/Articles/Classes/} ) {
my $tabs = PageMenu();
- if ( my $id = $m->request_args->{'id'} ) {
+ if ( my $id = $DECODED_ARGS->{'id'} ) {
my $obj = RT::CustomField->new( $session{'CurrentUser'} );
$obj->Load($id);
@@ -490,7 +490,7 @@ my $build_main_nav = sub {
$about_me->child( logout => title => loc('Logout'), path => '/NoAuth/Logout.html' );
}
if ( $request_path =~ m{^/Dashboards/(\d+)?}) {
- if ( my $id = ( $1 || $m->request_args->{'id'} ) ) {
+ if ( my $id = ( $1 || $DECODED_ARGS->{'id'} ) ) {
my $obj = RT::Dashboard->new( $session{'CurrentUser'} );
$obj->LoadById($id);
if ( $obj and $obj->id ) {
@@ -506,7 +506,7 @@ my $build_main_nav = sub {
if ( $request_path =~ m{^/Ticket/} ) {
- if ( ( $m->request_args->{'id'} || '' ) =~ /^(\d+)$/ ) {
+ if ( ( $DECODED_ARGS->{'id'} || '' ) =~ /^(\d+)$/ ) {
my $id = $1;
my $obj = RT::Ticket->new( $session{'CurrentUser'} );
$obj->Load($id);
@@ -654,17 +654,17 @@ my $build_main_nav = sub {
&& $request_path !~ m{^/Search/Simple\.html}
)
|| ( $request_path =~ m{^/Search/Simple\.html}
- && $m->request_args->{'q'} )
+ && $DECODED_ARGS->{'q'} )
)
{
my $search = Menu()->child('search');
my $args = '';
my $has_query = '';
my $current_search = $session{"CurrentSearchHash"} || {};
- my $search_id = $m->request_args->{'SavedSearchLoad'} || $m->request_args->{'SavedSearchId'} || $current_search->{'SearchId'} || '';
- my $chart_id = $m->request_args->{'SavedChartSearchId'} || $current_search->{SavedChartSearchId};
+ my $search_id = $DECODED_ARGS->{'SavedSearchLoad'} || $DECODED_ARGS->{'SavedSearchId'} || $current_search->{'SearchId'} || '';
+ my $chart_id = $DECODED_ARGS->{'SavedChartSearchId'} || $current_search->{SavedChartSearchId};
- $has_query = 1 if ( $m->request_args->{'Query'} or $current_search->{'Query'} );
+ $has_query = 1 if ( $DECODED_ARGS->{'Query'} or $current_search->{'Query'} );
my %query_args;
my %fallback_query_args = (
@@ -673,12 +673,12 @@ my $build_main_nav = sub {
(
map {
my $p = $_;
- $p => $m->request_args->{$p} || $current_search->{$p}
+ $p => $DECODED_ARGS->{$p} || $current_search->{$p}
} qw(Query Format OrderBy Order Page)
),
RowsPerPage => (
- defined $m->request_args->{'RowsPerPage'}
- ? $m->request_args->{'RowsPerPage'}
+ defined $DECODED_ARGS->{'RowsPerPage'}
+ ? $DECODED_ARGS->{'RowsPerPage'}
: $current_search->{'RowsPerPage'}
),
);
@@ -773,8 +773,8 @@ my $build_main_nav = sub {
}
if ( $request_path =~ m{^/Article/} ) {
- if ( $m->request_args->{'id'} && $m->request_args->{'id'} =~ /^\d+$/ ) {
- my $id = $m->request_args->{'id'};
+ if ( $DECODED_ARGS->{'id'} && $DECODED_ARGS->{'id'} =~ /^\d+$/ ) {
+ my $id = $DECODED_ARGS->{'id'};
my $tabs = PageMenu();
$tabs->child( display => title => loc('Display'), path => "/Articles/Article/Display.html?id=".$id );
@@ -788,7 +788,7 @@ my $build_main_nav = sub {
my $tabs = PageMenu();
$tabs->child( search => title => loc("Search"), path => "/Articles/Article/Search.html" );
$tabs->child( create => title => loc("New Article" ), path => "/Articles/Article/PreCreate.html" );
- if ( $request_path =~ m{^/Articles/Article/} and ( $m->request_args->{'id'} || '' ) =~ /^(\d+)$/ ) {
+ if ( $request_path =~ m{^/Articles/Article/} and ( $DECODED_ARGS->{'id'} || '' ) =~ /^(\d+)$/ ) {
my $id = $1;
my $obj = RT::Article->new( $session{'CurrentUser'} );
$obj->Load($id);
commit 7eaa035980cabbcb21fdbc92d9b8b4691cd735a8
Merge: 6dc6a0b 5c96ad5
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Fri May 4 16:15:09 2012 -0400
Merge branch 'security/4.0/interstitial-path' into 4.0.6-releng
commit 48ff24953c5af2efd77fe0f80490cd98aa31eb0f
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Fri May 4 16:49:31 2012 -0400
Clean up the error message in a common case of no explicit whitelisted hosts
diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index 1919b12..2c2f625 100644
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -1276,11 +1276,18 @@ EOT
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]) or whitelisted hosts ([_3])", # loc
+ "the Referrer header supplied by your browser ([_1]) is not allowed by RT's configured hostname ([_2])", # loc
$browser->host_port,
- shift @$configs,
- join(', ', @$configs) );
+ $configs->[0]);
}
sub ExpandCSRFToken {
commit 843659b101f4aecc0fcae17dac7dd2206356ec73
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Thu May 3 23:48:36 2012 -0400
Set the refresh URL on ticket results to a CRSF-safe one
Unfortunately, browsers do not always provide a referrer when
redirecting to a page by way of a <meta http-equiv="refresh">. As such,
automatic result refreshes trigger CSRF protections, as they request a
complex URL with many query parameters, and no referrer.
Work around this by generating a CSRF token which encodes the complete
set of query parameters, and redirecting to that. An unfortunate but
unavoidable side effect of this is that the session is bloated by each
of these sets of store query parameters on each search result page that
has a refresh set.
diff --git a/share/html/Search/Results.html b/share/html/Search/Results.html
index 0040d2a..a83e62a 100755
--- a/share/html/Search/Results.html
+++ b/share/html/Search/Results.html
@@ -46,7 +46,7 @@
%#
%# END BPS TAGGED BLOCK }}}
<& /Elements/Header, Title => $title,
- Refresh => $session{'tickets_refresh_interval'} || RT->Config->Get('SearchResultsRefreshInterval', $session{'CurrentUser'} ),
+ Refresh => $refresh,
LinkRel => \%link_rel &>
<& /Elements/Tabs &>
<& /Elements/CollectionList,
@@ -148,6 +148,16 @@ if ($ARGS{'TicketsRefreshInterval'}) {
$session{'tickets_refresh_interval'} = $ARGS{'TicketsRefreshInterval'};
}
+my $refresh = $session{'tickets_refresh_interval'}
+ || RT->Config->Get('SearchResultsRefreshInterval', $session{'CurrentUser'} );
+
+if ($refresh and not $m->request_args->{CSRF_Token}) {
+ my $token = RT::Interface::Web::StoreRequestToken( $session{'CurrentSearchHash'} );
+ $m->notes->{RefreshURL} = RT->Config->Get('WebURL')
+ . "Search/Results.html?CSRF_Token="
+ . $token;
+}
+
my %link_rel;
my $genpage = sub {
return $m->comp(
commit 58c006e6719a4a93e5422074dafdbd90f0bb2a48
Merge: 7eaa035 843659b
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Fri May 4 17:09:11 2012 -0400
Merge branch 'security/4.0/refresh-csrf' into 4.0.6-releng
diff --cc lib/RT/Interface/Web.pm
index 0f70ceb,6e2cee7..0bb3ddd
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@@ -1337,10 -1330,28 +1330,28 @@@ sub StoreRequestToken
$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->path_info,
+ OriginalURL => RT->Config->Get('WebPath') . $HTML::Mason::Commands::r->path_info,
Reason => HTML::Mason::Commands::loc( $msg, @loc ),
Token => $token,
);
commit 730eea81fec59cffbc3b9631b2b225c99a6cb704
Merge: 58c006e 33840d6
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Fri May 4 17:09:29 2012 -0400
Merge branch 'security/4.0/csrf-menuing' into 4.0.6-releng
commit b770e5f8abc6418ca8cb8e592287af535bd72249
Merge: 730eea8 48ff249
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Fri May 4 17:09:31 2012 -0400
Merge branch 'security/4.0/whitelist-csrf-referrer' into 4.0.6-releng
commit 096e31e8f7cffcaea01b3aed91355181fee8b0bd
Author: Kevin Falcone <falcone at bestpractical.com>
Date: Fri May 4 23:44:43 2012 -0400
Teach RT->Config->Set() how to handle ReferrerWhitelist
Without this, you can't push onto the list in ReferrerWhitelist
because Set just assumes that the config option is a SCALAR.
diff --git a/lib/RT/Config.pm b/lib/RT/Config.pm
index e17ad37..301b9f5 100644
--- a/lib/RT/Config.pm
+++ b/lib/RT/Config.pm
@@ -610,6 +610,7 @@ our %META = (
}
}
},
+ ReferrerWhitelist => { Type => 'ARRAY' },
ResolveDefaultUpdateType => {
PostLoadCheck => sub {
my $self = shift;
commit 299b6604bd36c99bbffa0710a239f3ec4f60e03a
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Mon May 7 00:29:29 2012 -0400
Only enable CSRF argument stashing in refresh URL if CSRF is enabled
Not only is it only necessary if CSRF protections are on, but RT does
not expand CSRF_Token unless RestrictReferrer is enabled.
diff --git a/share/html/Search/Results.html b/share/html/Search/Results.html
index a83e62a..171b38d 100755
--- a/share/html/Search/Results.html
+++ b/share/html/Search/Results.html
@@ -151,7 +151,7 @@ if ($ARGS{'TicketsRefreshInterval'}) {
my $refresh = $session{'tickets_refresh_interval'}
|| RT->Config->Get('SearchResultsRefreshInterval', $session{'CurrentUser'} );
-if ($refresh and not $m->request_args->{CSRF_Token}) {
+if (RT->Config->Get('RestrictReferrer') and $refresh and not $m->request_args->{CSRF_Token}) {
my $token = RT::Interface::Web::StoreRequestToken( $session{'CurrentSearchHash'} );
$m->notes->{RefreshURL} = RT->Config->Get('WebURL')
. "Search/Results.html?CSRF_Token="
commit ddb3ab99a6eb359394a6d9c9b5ec4d471c061601
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Mon May 14 15:21:11 2012 -0400
AddAttachments must use RT->SystemUser when searching for attachments to use
c29107c changed AddAttachments to use the transaction's current user to
search for which attachments to add to the outgoing mail.
Unfortunately, this ignored the common case where the transaction's
current user is an unprivileged user who does not have rights to see
their own attachment. This manifested itself as AdminCc emails not
having attachments which were included with the original mail that
triggered them, despite RT-Attach-Message being set.
Revert the CurrentUser on the Attachments search to RT->SystemUser, as
it was pre- c29107c. This does not re-open the vulnerability, as
(unlike the AddTicket functionality) the transaction creator can only
cause attachments on their own transaction to be distributed. While one
route to fix this would be to modify RT::Attachments->Next to allow
creators to always see their own attachments, such a change might have
broader-reaching implications.
diff --git a/lib/RT/Action/SendEmail.pm b/lib/RT/Action/SendEmail.pm
index 94686b8..4ae1a8b 100644
--- a/lib/RT/Action/SendEmail.pm
+++ b/lib/RT/Action/SendEmail.pm
@@ -348,7 +348,7 @@ sub AddAttachments {
$MIMEObj->head->delete('RT-Attach-Message');
- my $attachments = RT::Attachments->new( $self->TransactionObj->CreatorObj );
+ my $attachments = RT::Attachments->new( RT->SystemUser );
$attachments->Limit(
FIELD => 'TransactionId',
VALUE => $self->TransactionObj->Id
commit 650e03250271a39121eba428a41b1592d8342a79
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Mon May 14 17:56:47 2012 -0400
Ensure that updated session is sent to clients after external auth
While 162cd06 correctly identified that sending the cookie was only
necessary after reading it or re-auth, it failed to notice that
InstantiateNewSession is called elsewhere than
AttemptPasswordAuthentication (notably AttemptExternalAuth), all of
which require SendSessionCookie calls to function correctly.
Ensure that the updated cookie value is always set after it is changed
by InstantiateNewSession, as well as directly before page display (in
case other callbacks change the session id by other means).
diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index d0542fe..c8b258f 100644
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -540,6 +540,10 @@ 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();
+
# precache all system level rights for the current user
$HTML::Mason::Commands::session{CurrentUser}->PrincipalObj->HasRights( Object => RT->System );
@@ -691,7 +695,6 @@ sub AttemptPasswordAuthentication {
InstantiateNewSession();
$HTML::Mason::Commands::session{'CurrentUser'} = $user_obj;
- SendSessionCookie();
$m->callback( %$ARGS, CallbackName => 'SuccessfulLogin', CallbackPage => '/autohandler' );
@@ -746,6 +749,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 {
commit 153a17f9e898a744eeec45c983cdccf0055b22ea
Merge: 4389336 650e032
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Tue May 22 12:01:36 2012 -0400
Merge branch '4.0.6-releng' into 4.0-trunk
-----------------------------------------------------------------------
More information about the Rt-commit
mailing list