From 70e9b59d228face74faea3f9440a48a4092c76bc Mon Sep 17 00:00:00 2001 From: skodak Date: Sun, 16 Sep 2007 20:19:19 +0000 Subject: [PATCH] MDL-11300 1/ potential sql injection through tag types - missing param type + can add new types + usually no addslashes before update of tags 2/ paging does not work at all 3/ missing param type definitions 4/ not xhtml strict compliant 5/ CSRF - missing sesskey checks 6/ UI code in tag/lib.php - should be in manage.php or other file 7/ missing localization of types - default/standard --- lang/en_utf8/tag.php | 2 + tag/lib.php | 170 ++-------------------------- tag/manage.php | 260 +++++++++++++++++++++++++++++++++++-------- 3 files changed, 222 insertions(+), 210 deletions(-) diff --git a/lang/en_utf8/tag.php b/lang/en_utf8/tag.php index 6e3492e8b1b..1daf7472cb9 100644 --- a/lang/en_utf8/tag.php +++ b/lang/en_utf8/tag.php @@ -32,6 +32,8 @@ $string['select'] = 'Select'; $string['tagdescription'] = 'Tag description'; $string['tag'] = 'Tag'; $string['tagtype'] = 'Tag type'; +$string['tagtype_official'] = 'Official'; +$string['tagtype_default'] = 'Default'; $string['tags'] = 'Tags'; $string['tagsaredisabled'] = 'Tags are disabled'; $string['thingstaggedwith'] = '$a->count things tagged with \"$a->name\"'; diff --git a/tag/lib.php b/tag/lib.php index e15f2061b08..f234db58d9b 100644 --- a/tag/lib.php +++ b/tag/lib.php @@ -121,9 +121,10 @@ function tag_exists($tag_name_or_id) { if (is_numeric($tag_name_or_id)) { return record_exists('tag', 'id', $tag_name_or_id); - } - elseif (is_string($tag_name_or_id)) { - return record_exists('tag', 'name', $tag_name_or_id); + + } else { + // normalised tag names are always lowercase only + return record_exists('tag', 'name', moodle_strtolower($tag_name_or_id)); } } @@ -157,11 +158,12 @@ function tags_id($tag_names_csv) { $normalized_tag_names_csv = tag_normalize($tag_names_csv); $tag_names_csv_with_apos = "'" . str_replace(',', "','", $normalized_tag_names_csv ) . "'"; - $tag_objects = get_records_list('tag','name', $tag_names_csv_with_apos, "" , "name, id" ); - $tags_ids = array(); - foreach ($tag_objects as $tag) { - $tags_ids[$tag->name] = $tag->id; + + if ($tag_objects = get_records_list('tag','name', $tag_names_csv_with_apos, "" , "name, id" )) { + foreach ($tag_objects as $tag) { + $tags_ids[$tag->name] = $tag->id; + } } return $tags_ids; @@ -1545,158 +1547,4 @@ function print_tag_cloud($tagcloud, $shuffle=true, $max_size=180, $min_size=80, } -function print_tag_management_list($perpage='100') { - - global $CFG, $USER; - require_once($CFG->libdir.'/tablelib.php'); - - //setup table - - $tablecolumns = array('id','name', 'fullname', 'count', 'flag', 'timemodified', 'rawname', 'tagtype', ''); - $tableheaders = array( get_string('id' , 'tag'), - get_string('name' , 'tag'), - get_string('owner','tag'), - get_string('count','tag'), - get_string('flag','tag'), - get_string('timemodified','tag'), - get_string('newname', 'tag'), - get_string('tagtype', 'tag'), - get_string('select', 'tag') - ); - - $table = new flexible_table('tag-management-list-'.$USER->id); - - $baseurl = $CFG->wwwroot.'/tag/manage.php'; - - $table->define_columns($tablecolumns); - $table->define_headers($tableheaders); - $table->define_baseurl($baseurl); - - $table->sortable(true, 'flag', SORT_DESC); - - $table->set_attribute('cellspacing', '0'); - $table->set_attribute('id', 'tag-management-list'); - $table->set_attribute('class', 'generaltable generalbox'); - - $table->set_control_variables(array( - TABLE_VAR_SORT => 'ssort', - TABLE_VAR_HIDE => 'shide', - TABLE_VAR_SHOW => 'sshow', - TABLE_VAR_IFIRST => 'sifirst', - TABLE_VAR_ILAST => 'silast', - TABLE_VAR_PAGE => 'spage' - )); - - $table->setup(); - - if ($table->get_sql_sort()) { - $sort = ' ORDER BY '.$table->get_sql_sort(); - } else { - $sort = ''; - } - - if ($table->get_sql_where()) { - $where = 'WHERE '.$table->get_sql_where(); - } else { - $where = ''; - } - - $query = " - SELECT - tg.id, tg.name, tg.rawname, tg.tagtype, COUNT(ti.id) AS count, u.id AS owner, tg.flag, tg.timemodified, u.firstname, u.lastname - FROM - {$CFG->prefix}tag_instance ti - RIGHT JOIN - {$CFG->prefix}tag tg - ON - tg.id = ti.tagid - LEFT JOIN - {$CFG->prefix}user u - ON - tg.userid = u.id - {$where} - GROUP BY - tg.id - {$sort} - "; - - - $totalcount = count_records_sql("SELECT COUNT(DISTINCT(tg.id)) - FROM {$CFG->prefix}tag tg LEFT JOIN {$CFG->prefix}user u ON u.id = tg.userid - $where"); - - $table->initialbars(true); // always initial bars - $table->pagesize($perpage, $totalcount); - - echo '
'; - - //retrieve tags from DB - if ($tagrecords = get_records_sql($query, $table->get_page_start(), $table->get_page_size())) { - - $taglist = array_values($tagrecords); - - //print_tag_cloud(array_values(get_records_sql($query)), false); - - //populate table with data - foreach ($taglist as $tag ){ - - $id = $tag->id; - $name = ''. tag_display_name($tag) .''; - $owner = '' . fullname($tag) . ''; - $count = $tag->count; - $flag = $tag->flag; - $timemodified = format_time(time() - $tag->timemodified); - $checkbox = ''; - $text = ''; - - // get all the possible tag types from db - $tagtypes = array(); - if ($ptypes = get_records_sql("SELECT DISTINCT(tagtype), id FROM {$CFG->prefix}tag")) { - foreach ($ptypes as $ptype) { - $tagtypes[$ptype->tagtype] = $ptype->tagtype; - } - } - // default types - $tagtypes['default']='default'; - $tagtypes['official']='official'; - - $tagtype = choose_from_menu ($tagtypes, 'tagtypes['.$tag->id.']', $tag->tagtype, '', '', '0', true); - - //if the tag if flagged, highlight it - if ($tag->flag > 0) { - $id = '' . $id . ''; - $name = '' . $name . ''; - $owner = '' . $owner . ''; - $count = '' . $count . ''; - $flag = '' . $flag . ''; - $timemodified = '' . $timemodified . ''; - $tagtype = ''. $tagtype. ''; - } - - $data = array($id, $name , $owner ,$count ,$flag, $timemodified, $text, $tagtype, $checkbox); - - $table->add_data($data); - } - - - echo ' '; - echo ' '; - echo '

'; - echo ''; - - echo ''; - - } - - $table->print_html(); - - echo '
'; -} - ?> diff --git a/tag/manage.php b/tag/manage.php index c91a5c6f8fd..b8250775a7a 100644 --- a/tag/manage.php +++ b/tag/manage.php @@ -1,12 +1,21 @@ libdir.'/tablelib.php'); require_once('lib.php'); -require_once($CFG->dirroot.'/lib/weblib.php'); + +define('SHOW_ALL_PAGE_SIZE', 50000); +define('DEFAULT_PAGE_SIZE', 30); + +$tagschecked = optional_param('tagschecked', array(), PARAM_INT); +$newnames = optional_param('newname', array(), PARAM_TAG); +$tagtypes = optional_param('tagtypes', array(), PARAM_ALPHA); +$action = optional_param('action', '', PARAM_ALPHA); +$perpage = optional_param('perpage', DEFAULT_PAGE_SIZE, PARAM_INT); require_login(); -if( empty($CFG->usetags)) { +if (empty($CFG->usetags)) { error(get_string('tagsaredisabled', 'tag')); } @@ -14,11 +23,6 @@ if( empty($CFG->usetags)) { $systemcontext = get_context_instance(CONTEXT_SYSTEM); require_capability('moodle/tag:manage', $systemcontext); -$tagschecked = optional_param('tagschecked', array()); -$newnames = optional_param('newname', array()); -$tagtypes = optional_param('tagtypes', array()); -$action = optional_param('action', '', PARAM_ALPHA); - $navlinks = array(); $navlinks[] = array('name' => get_string('tags', 'tag'), 'link' => "{$CFG->wwwroot}/tag/search.php", 'type' => ''); $navlinks[] = array('name' => get_string('managetags', 'tag'), 'link' => '', 'type' => ''); @@ -26,72 +30,98 @@ $navlinks[] = array('name' => get_string('managetags', 'tag'), 'link' => '', 'ty $navigation = build_navigation($navlinks); print_header_simple(get_string('managetags', 'tag'), '', $navigation); -$notice = tag_name_from_string(implode($tagschecked, ', ')); -$notice = str_replace(',', ', ', $notice); - $err_notice = ''; +$notice = ''; + +// get all the possible tag types from db +$existing_tagtypes = array(); +if ($ptypes = get_records_sql("SELECT DISTINCT(tagtype), id FROM {$CFG->prefix}tag")) { + foreach ($ptypes as $ptype) { + $existing_tagtypes[$ptype->tagtype] = $ptype->tagtype; + } +} +$existing_tagtypes['official'] = get_string('tagtype_official', 'tag'); +$existing_tagtypes['default'] = get_string('tagtype_default', 'tag'); switch($action) { case 'delete': - - $notice .= ' -- ' . get_string('deleted','tag'); + if (!data_submitted or !confirm_sesskey()) { + break; + } + $str_tagschecked = tag_name_from_string(implode($tagschecked, ',')); + $str_tagschecked = str_replace(',', ', ', $str_tagschecked); tag_delete(implode($tagschecked, ',')); - - break; - - case 'reset': - $notice .= ' -- ' . get_string('reset','tag'); - - tag_flag_reset(implode($tagschecked, ',')); - + $notice = $str_tagschecked.' -- '.get_string('deleted','tag'); break; + + case 'reset': + if (!data_submitted or !confirm_sesskey()) { + break; + } + $str_tagschecked = tag_name_from_string(implode($tagschecked, ',')); + $str_tagschecked = str_replace(',', ', ', $str_tagschecked); + + tag_flag_reset(implode($tagschecked, ',')); + + $notice = $str_tagschecked.' -- '.get_string('reset','tag'); + break; + case 'changetype': - - foreach ($tagschecked as $tag_id){ - + if (!data_submitted or !confirm_sesskey()) { + break; + } + + $changed = array(); + + foreach ($tagschecked as $tag_id) { + if (!in_array($tagtypes[$tag_id], $existing_tagtypes)) { + //can not add new types here!! + continue; + } + // update tag type; $tag = tag_by_id($tag_id); - - $tag -> tagtype = $tagtypes[$tag_id]; + $tag->tagtype = $tagtypes[$tag_id]; + if (update_record('tag', $tag)) { - $notice .= ' -- ' . get_string('typechanged','tag'); + $changed[] = $tag_id; } } - + + if ($changed) { + $str_changed = tag_name_from_string(implode($changed, ',')); + $str_changed = str_replace(',', ', ', $str_changed); + $notice = $str_changed.' -- '.get_string('typechanged','tag'); + } break; + case 'changename': - - $normalized_new_names_csv = tag_normalize( str_replace(',,','',implode($newnames, ',')) ); - - //tag names entered might already exist - $existing_tags = tags_id( $normalized_new_names_csv ); - - //notice to warn that names already exist - foreach ($existing_tags as $name => $tag){ - $err_notice .= $name . ', '; + if (!data_submitted or !confirm_sesskey()) { + break; } - if(!empty($err_notice)){ - $err_notice .= '-- ' . get_string('namesalreadybeeingused','tag'); - } - - - //update tag names with the names passed in $newnames + $tags_names_changed = array(); - foreach ($tagschecked as $tag_id){ - $tags_names_changed[$tag_id] = str_replace(',','',$newnames[$tag_id]) ; + + foreach ($tagschecked as $tag_id) { + if ($newnames[$tag_id] != '') { + if (tag_exists($newnames[$tag_id])) { + $err_notice .= $newnames[$tag_id]. '-- ' . get_string('namesalreadybeeingused','tag').'
'; + } else { + $tags_names_changed[$tag_id] = $newnames[$tag_id]; + } + } } $tags_names_updated = tag_update_name($tags_names_changed); - + //notice to inform what tags had their names effectively updated - $notice = implode($tags_names_updated, ', '); - if(!empty($notice)){ + if ($tags_names_updated){ + $notice = implode($tags_names_updated, ', '); $notice .= ' -- ' . get_string('updated','tag'); } - break; } @@ -104,8 +134,140 @@ if ($notice) { notify($notice , 'green'); } -print_tag_management_list(); +//setup table +$tablecolumns = array('id','name', 'fullname', 'count', 'flag', 'timemodified', 'rawname', 'tagtype', ''); +$tableheaders = array(get_string('id' , 'tag'), + get_string('name' , 'tag'), + get_string('owner','tag'), + get_string('count','tag'), + get_string('flag','tag'), + get_string('timemodified','tag'), + get_string('newname', 'tag'), + get_string('tagtype', 'tag'), + get_string('select', 'tag')); + +$table = new flexible_table('tag-management-list-'.$USER->id); + +$baseurl = $CFG->wwwroot.'/tag/manage.php?perpage='.$perpage; + +$table->define_columns($tablecolumns); +$table->define_headers($tableheaders); +$table->define_baseurl($baseurl); + +$table->sortable(true, 'flag', SORT_DESC); + +$table->set_attribute('cellspacing', '0'); +$table->set_attribute('id', 'tag-management-list'); +$table->set_attribute('class', 'generaltable generalbox'); + +$table->set_control_variables(array( +TABLE_VAR_SORT => 'ssort', +TABLE_VAR_HIDE => 'shide', +TABLE_VAR_SHOW => 'sshow', +TABLE_VAR_IFIRST => 'sifirst', +TABLE_VAR_ILAST => 'silast', +TABLE_VAR_PAGE => 'spage' +)); + +$table->setup(); + +if ($table->get_sql_sort()) { + $sort = ' ORDER BY '.$table->get_sql_sort(); +} else { + $sort = ''; +} + +if ($table->get_sql_where()) { + $where = 'WHERE '.$table->get_sql_where(); +} else { + $where = ''; +} + +$query = " + SELECT tg.id, tg.name, tg.rawname, tg.tagtype, COUNT(ti.id) AS count, u.id AS owner, tg.flag, tg.timemodified, u.firstname, u.lastname + FROM {$CFG->prefix}tag_instance ti + RIGHT JOIN {$CFG->prefix}tag tg ON tg.id = ti.tagid + LEFT JOIN {$CFG->prefix}user u ON tg.userid = u.id + {$where} + GROUP BY tg.id + {$sort}"; + + +$totalcount = count_records_sql("SELECT COUNT(DISTINCT(tg.id)) + FROM {$CFG->prefix}tag tg + LEFT JOIN {$CFG->prefix}user u ON u.id = tg.userid + $where"); + +$table->initialbars(true); // always initial bars +$table->pagesize($perpage, $totalcount); + +echo '
'; + +//retrieve tags from DB +if ($tagrecords = get_records_sql($query, $table->get_page_start(), $table->get_page_size())) { + + $taglist = array_values($tagrecords); + + //print_tag_cloud(array_values(get_records_sql($query)), false); + + //populate table with data + foreach ($taglist as $tag ){ + + $id = $tag->id; + $name = ''. tag_display_name($tag) .''; + $owner = '' . fullname($tag) . ''; + $count = $tag->count; + $flag = $tag->flag; + $timemodified = format_time(time() - $tag->timemodified); + $checkbox = ''; + $text = ''; + + $tagtype = choose_from_menu($existing_tagtypes, 'tagtypes['.$tag->id.']', $tag->tagtype, '', '', '0', true); + + //if the tag if flagged, highlight it + if ($tag->flag > 0) { + $id = '' . $id . ''; + $name = '' . $name . ''; + $owner = '' . $owner . ''; + $count = '' . $count . ''; + $flag = '' . $flag . ''; + $timemodified = '' . $timemodified . ''; + $tagtype = ''. $tagtype. ''; + } + + $data = array($id, $name, $owner, $count, $flag, $timemodified, $text, $tagtype, $checkbox); + + $table->add_data($data); + } + + + echo ' '; + echo ' '; + echo ' '; + echo '

'; + echo ''; + + echo ''; + +} + +$table->print_html(); + +echo '
'; + +if ($perpage == SHOW_ALL_PAGE_SIZE) { + echo '
'.get_string('showperpage', '', DEFAULT_PAGE_SIZE).'
'; + +} else if ($totalcount > 0 and $perpage < $totalcount) { + echo '
'.get_string('showall', '', $totalcount).'
'; +} echo '
';