Skip to content

Commit

Permalink
Override $DECODED_ARGS with the (decoded) arguments from the CSRF token
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@alexmv
alexmv committed May 4, 2012
1 parent 17bc0c1 commit 33840d6
Show file tree
Hide file tree
Showing 2 changed files with 24 additions and 23 deletions.
1 change: 1 addition & 0 deletions lib/RT/Interface/Web.pm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
46 changes: 23 additions & 23 deletions share/html/Elements/Tabs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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);

Expand Down Expand Up @@ -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);

Expand All @@ -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);

Expand All @@ -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);

Expand All @@ -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);

Expand Down Expand Up @@ -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 ) {
Expand All @@ -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);
Expand Down Expand Up @@ -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 = (
Expand All @@ -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'}
),
);
Expand Down Expand Up @@ -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 );
Expand All @@ -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);
Expand Down

0 comments on commit 33840d6

Please sign in to comment.