From 05ca6dadf8b9b4db3c06f4b3207d2cca4d5e603f Mon Sep 17 00:00:00 2001 From: Andrew Nicols Date: Fri, 13 Dec 2013 10:53:26 +0800 Subject: [PATCH 1/2] MDL-34182: Add functions to warn of output in AJAX scripts We need to be able to capture output when including contributed code as it may add additional content which makes makes the json we output invalid. We only warn of these errors if the site is in debugdeveloper, otherwise we silently drop them. --- lib/ajax/ajaxlib.php | 37 ++++++++++++ lib/tests/ajaxlib_test.php | 121 +++++++++++++++++++++++++++++++++++++ 2 files changed, 158 insertions(+) create mode 100644 lib/tests/ajaxlib_test.php diff --git a/lib/ajax/ajaxlib.php b/lib/ajax/ajaxlib.php index 4c298b54b59..7fe4b034969 100644 --- a/lib/ajax/ajaxlib.php +++ b/lib/ajax/ajaxlib.php @@ -80,3 +80,40 @@ function ajaxenabled(array $browsers = null) { return false; } } + +/** + * Starts capturing output whilst processing an AJAX request. + * + * This should be used in combination with ajax_check_captured_output to + * report any captured output to the user. + * + * @retrun Boolean Returns true on success or false on failure. + */ +function ajax_capture_output() { + // Start capturing output in case of broken plugins. + return ob_start(); +} + +/** + * Check captured output for content. If the site has a debug level of + * debugdeveloper set, and the content is non-empty, then throw a coding + * exception which can be captured by the Y.IO request and displayed to the + * user. + * + * @return Any output that was captured. + */ +function ajax_check_captured_output() { + global $CFG; + + // Retrieve the output - there should be none. + $output = ob_get_contents(); + ob_end_clean(); + + if ($CFG->debug == DEBUG_DEVELOPER && !empty($output)) { + // Only throw an error if the site is in debugdeveloper. + throw new coding_exception('Unexpected output whilst processing AJAX request. ' . + 'This could be caused by trailing whitespace. Output received: ' . + var_export($output, true)); + } + return $output; +} diff --git a/lib/tests/ajaxlib_test.php b/lib/tests/ajaxlib_test.php new file mode 100644 index 00000000000..9baea2ce8fb --- /dev/null +++ b/lib/tests/ajaxlib_test.php @@ -0,0 +1,121 @@ +. + +/** + * Code quality unit tests that are fast enough to run each time. + * + * @package core + * @category phpunit + * @copyright 2013 Andrew Nicols + * @license http://www.gnu.org/copyleft/gpl.html GNU Public License + */ + +defined('MOODLE_INTERNAL') || die(); + +class core_ajaxlib_testcase extends advanced_testcase { + + protected function helper_test_clean_output() { + $this->resetAfterTest(); + + $result = ajax_capture_output(); + + // ob_start should normally return without issue. + $this->assertTrue($result); + + $result = ajax_check_captured_output(); + $this->assertEmpty($result); + } + + protected function helper_test_dirty_output($expectexception = false) { + $this->resetAfterTest(); + + // Keep track of the content we will output. + $content = "Some example content"; + + $result = ajax_capture_output(); + + // ob_start should normally return without issue. + $this->assertTrue($result); + + // Fill the output buffer. + echo $content; + + if ($expectexception) { + $this->setExpectedException('coding_exception'); + ajax_check_captured_output(); + } else { + $result = ajax_check_captured_output(); + $this->assertEquals($result, $content); + } + } + + public function test_output_capture_normal_debug_none() { + global $CFG; + // In normal conditions, and with DEBUG_NONE set, we should not receive any output or throw any exceptions. + $CFG->debug = DEBUG_NONE; + $this->helper_test_clean_output(); + } + + public function test_output_capture_normal_debug_normal() { + global $CFG; + // In normal conditions, and with DEBUG_NORMAL set, we should not receive any output or throw any exceptions. + $CFG->debug = DEBUG_NORMAL; + $this->helper_test_clean_output(); + } + + public function test_output_capture_normal_debug_all() { + global $CFG; + // In normal conditions, and with DEBUG_ALL set, we should not receive any output or throw any exceptions. + $CFG->debug = DEBUG_ALL; + $this->helper_test_clean_output(); + } + + public function test_output_capture_normal_debugdeveloper() { + global $CFG; + // In normal conditions, and with DEBUG_DEVELOPER set, we should not receive any output or throw any exceptions. + $CFG->debug = DEBUG_DEVELOPER; + $this->helper_test_clean_output(); + } + + public function test_output_capture_error_debug_none() { + global $CFG; + // With DEBUG_NONE set, we should not throw any exception, but the output will be returned. + $CFG->debug = DEBUG_NONE; + $this->helper_test_dirty_output(); + } + + public function test_output_capture_error_debug_normal() { + global $CFG; + // With DEBUG_NORMAL set, we should not throw any exception, but the output will be returned. + $CFG->debug = DEBUG_NORMAL; + $this->helper_test_dirty_output(); + } + + public function test_output_capture_error_debug_all() { + global $CFG; + // In error conditions, and with DEBUG_ALL set, we should not receive any output or throw any exceptions. + $CFG->debug = DEBUG_ALL; + $this->helper_test_dirty_output(); + } + + public function test_output_capture_error_debugdeveloper() { + global $CFG; + // With DEBUG_DEVELOPER set, we should throw an exception. + $CFG->debug = DEBUG_DEVELOPER; + $this->helper_test_dirty_output(true); + } + +} From 05c42f75399457ca4939350eb2b0428c7b9333e3 Mon Sep 17 00:00:00 2001 From: Andrew Nicols Date: Fri, 13 Dec 2013 11:06:17 +0800 Subject: [PATCH 2/2] MDL-34182 Repositories: Check for and warn on invalid output in the file picker --- repository/filepicker.js | 24 ++++++++++-------------- repository/repository_ajax.php | 14 ++++++++++++++ 2 files changed, 24 insertions(+), 14 deletions(-) diff --git a/repository/filepicker.js b/repository/filepicker.js index 1beaf56fe22..afc23449d8d 100644 --- a/repository/filepicker.js +++ b/repository/filepicker.js @@ -583,27 +583,23 @@ M.core_filepicker.init = function(Y, options) { method: 'POST', on: { complete: function(id,o,p) { - if (!o) { - // TODO - alert('IO FATAL'); - return; - } var data = null; try { data = Y.JSON.parse(o.responseText); } catch(e) { - scope.print_msg(M.str.repository.invalidjson, 'error'); - scope.display_error(M.str.repository.invalidjson+'
'+stripHTML(o.responseText)+'
', 'invalidjson') - return; + if (o && o.status && o.status > 0) { + Y.use('moodle-core-notification', function() { + new M.core.exception(e); + }); + return; + } } // error checking if (data && data.error) { - scope.print_msg(data.error, 'error'); - if (args.onerror) { - args.onerror(id,data,p); - } else { - this.fpnode.one('.fp-content').setContent(''); - } + Y.use('moodle-core-notification', function() { + new M.core.ajaxException(data); + }); + this.fpnode.one('.fp-content').setContent(''); return; } else { if (data.msg) { diff --git a/repository/repository_ajax.php b/repository/repository_ajax.php index 7f6e4a641b7..fa8f488523f 100644 --- a/repository/repository_ajax.php +++ b/repository/repository_ajax.php @@ -75,6 +75,8 @@ $repooptions = array( 'ajax' => true, 'mimetypes' => $accepted_types ); + +ajax_capture_output(); $repo = repository::get_repository_by_id($repo_id, $contextid, $repooptions); // Check permissions @@ -128,6 +130,7 @@ switch ($action) { if ($repo->check_login()) { $listing = repository::prepare_listing($repo->get_listing($req_path, $page)); $listing['repo_id'] = $repo_id; + ajax_check_captured_output(); echo json_encode($listing); break; } else { @@ -136,23 +139,27 @@ switch ($action) { case 'login': $listing = $repo->print_login(); $listing['repo_id'] = $repo_id; + ajax_check_captured_output(); echo json_encode($listing); break; case 'logout': $logout = $repo->logout(); $logout['repo_id'] = $repo_id; + ajax_check_captured_output(); echo json_encode($logout); break; case 'searchform': $search_form['repo_id'] = $repo_id; $search_form['form'] = $repo->print_search(); $search_form['allowcaching'] = true; + ajax_check_captured_output(); echo json_encode($search_form); break; case 'search': $search_result = repository::prepare_listing($repo->search($search_text, (int)$page)); $search_result['repo_id'] = $repo_id; $search_result['issearchresult'] = true; + ajax_check_captured_output(); echo json_encode($search_result); break; case 'download': @@ -191,6 +198,7 @@ switch ($action) { $info['file'] = $saveas_filename; $info['type'] = 'link'; $info['url'] = $link; + ajax_check_captured_output(); echo json_encode($info); die; } else { @@ -282,6 +290,7 @@ switch ($action) { // You can cache reository file in this callback // or complete other tasks. $repo->cache_file_by_reference($reference, $storedfile); + ajax_check_captured_output(); echo json_encode($event); die; } else if ($repo->has_moodle_files()) { @@ -292,6 +301,7 @@ switch ($action) { // {@link repository::copy_to_area()}. $fileinfo = $repo->copy_to_area($reference, $record, $maxbytes, $areamaxbytes); + ajax_check_captured_output(); echo json_encode($fileinfo); die; } else { @@ -317,12 +327,14 @@ switch ($action) { $info['e'] = get_string('error', 'moodle'); } } + ajax_check_captured_output(); echo json_encode($info); die; } break; case 'upload': $result = $repo->upload($saveas_filename, $maxbytes); + ajax_check_captured_output(); echo json_encode($result); break; @@ -335,6 +347,7 @@ switch ($action) { $newfilename = required_param('newfilename', PARAM_FILE); $info = repository::overwrite_existing_draftfile($itemid, $filepath, $filename, $newfilepath, $newfilename); + ajax_check_captured_output(); echo json_encode($info); break; @@ -342,6 +355,7 @@ switch ($action) { // delete tmp file $newfilepath = required_param('newfilepath', PARAM_PATH); $newfilename = required_param('newfilename', PARAM_FILE); + ajax_check_captured_output(); echo json_encode(repository::delete_tempfile_from_draft($itemid, $newfilepath, $newfilename)); break;