Posted by doitDave on March 23, 2011 at 10:32pm
1 follower
| Project: | High-performance JavaScript callback handler |
| Version: | 6.x-2.x-dev |
| Component: | Code |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs review |
Issue Summary
1) what I came across.
I have set a callback where 'filter' module is in the dependencies list, because "my" module (actually I extended pmgrowl.module to benefit from js.module) uses filter functions. So far so bad. At the point where the module calls the check_markup function, filter_xss_bad_protocol is redeclared by filter.module. This makes js.php end with a parse error.
2) what I analyzed (at least what I'm thinking of as that)
The problem in the current dev release is that, when the filter_xss_bad_protocol clone is defined, it is not obvious whether this will be needed. Because if filter.module would be loaded anyway, that would be obsolete (correct me if I'm wrong, but that was what made up while I was testing). At the moment WHERE we know, the function is already defined, no way back.
3) how I worked it around
I split the js_execute_callback function into two phases. Phase 1 determines whether filter.module is to be loaded, phase 2 does the module loading as before. The definition of the helper function now takes place between these phases and depends on the result of phase 1.
Here's the diff, I will also attach it.
--- js.php.original Fr 25. Feb 02:08:40 2011
+++ js.php Mi 23. Mrz 23:10:55 2011
@@ -38,131 +38,154 @@
* @return
* The callback function's return value or one of the JS_* constants.
*/
-function js_execute_callback() {
+function js_execute_callback($phase=1) {
global $locale;
+
+ // remember between phases
+ static $full_bootstrap, $modules, $info, $args, $dependencies;
+
+ // first phase bootstraps, loads includes
+ // and prepares the module list to determine whether
+ // function filter_xss_bad_protocol needs to be redefined or not.
+
+ switch($phase) {
+ case 1:
+ $args = explode('/', $_GET['q']);
- $args = explode('/', $_GET['q']);
+ // Strip first argument 'js'.
+ array_shift($args);
- // Strip first argument 'js'.
- array_shift($args);
+ // Determine module to load.
+ $module = check_plain(array_shift($args));
+ if (!$module || !drupal_load('module', $module)) {
+ return JS_ACCESS_DENIED;
+ }
- // Determine module to load.
- $module = check_plain(array_shift($args));
- if (!$module || !drupal_load('module', $module)) {
- return JS_ACCESS_DENIED;
- }
+ // Get info hook function name.
+ $function = $module .'_js';
+ if (!function_exists($function)) {
+ return JS_NOT_FOUND;
+ }
- // Get info hook function name.
- $function = $module .'_js';
- if (!function_exists($function)) {
- return JS_NOT_FOUND;
- }
+ // Get valid callbacks.
+ $valid_callbacks = $function();
- // Get valid callbacks.
- $valid_callbacks = $function();
+ $callback = check_plain(array_shift($args));
+ if (!isset($valid_callbacks[$callback]) || !function_exists($valid_callbacks[$callback]['callback'])) {
+ return JS_NOT_FOUND;
+ }
- $callback = check_plain(array_shift($args));
- if (!isset($valid_callbacks[$callback]) || !function_exists($valid_callbacks[$callback]['callback'])) {
- return JS_NOT_FOUND;
- }
+ $info = $valid_callbacks[$callback];
+ $full_boostrap = FALSE;
- $info = $valid_callbacks[$callback];
- $full_boostrap = FALSE;
+ // Bootstrap to required level.
+ if (!empty($info['bootstrap'])) {
+ drupal_bootstrap($info['bootstrap']);
+ $full_boostrap = ($info['bootstrap'] == DRUPAL_BOOTSTRAP_FULL);
+ }
- // Bootstrap to required level.
- if (!empty($info['bootstrap'])) {
- drupal_bootstrap($info['bootstrap']);
- $full_boostrap = ($info['bootstrap'] == DRUPAL_BOOTSTRAP_FULL);
- }
+ if (!$full_boostrap) {
+ // The following mimics the behavior of _drupal_bootstrap_full().
+ // @see _drupal_bootstrap_full(), common.inc
- if (!$full_boostrap) {
- // The following mimics the behavior of _drupal_bootstrap_full().
- // @see _drupal_bootstrap_full(), common.inc
+ // Load required include files.
+ if (isset($info['includes']) && is_array($info['includes'])) {
+ foreach ($info['includes'] as $include) {
+ if (file_exists("./includes/$include.inc")) {
+ require_once "./includes/$include.inc";
+ }
+ }
+ }
+ // Always load locale.inc.
+ require_once "./includes/locale.inc";
- // Load required include files.
- if (isset($info['includes']) && is_array($info['includes'])) {
- foreach ($info['includes'] as $include) {
- if (file_exists("./includes/$include.inc")) {
- require_once "./includes/$include.inc";
+ // Set the Drupal custom error handler.
+ set_error_handler('drupal_error_handler');
+ // Detect string handling method.
+ if (function_exists('unicode_check')) {
+ unicode_check();
}
+ // Undo magic quotes.
+ fix_gpc_magic();
+
+ // Load required modules.
+ $modules = array($module => 0);
+ if (isset($info['dependencies']) && is_array($info['dependencies'])) {
+ // Intersect list with active modules to avoid loading uninstalled ones.
+ $dependencies = array_intersect(module_list(TRUE, FALSE), $info['dependencies']);
+ }
+ // determine and return whether "filter" module is included:
+ return $dependencies['filter'] == 'filter';
}
- }
- // Always load locale.inc.
- require_once "./includes/locale.inc";
+ break; // phase 1
+ case 2:
+ if (!$full_bootstrap) {
+ if (is_array($dependencies) && !empty($dependencies)) {
+ foreach ($dependencies as $dependency) {
+ drupal_load('module', $dependency);
+ $modules[$dependency] = 0;
+ }
+ }
+ // Reset module list.
+ module_list(FALSE, TRUE, FALSE, $modules);
- // Set the Drupal custom error handler.
- set_error_handler('drupal_error_handler');
- // Detect string handling method.
- if (function_exists('unicode_check')) {
- unicode_check();
- }
- // Undo magic quotes.
- fix_gpc_magic();
-
- // Load required modules.
- $modules = array($module => 0);
- if (isset($info['dependencies']) && is_array($info['dependencies'])) {
- // Intersect list with active modules to avoid loading uninstalled ones.
- $dependencies = array_intersect(module_list(TRUE, FALSE), $info['dependencies']);
- foreach ($dependencies as $dependency) {
- drupal_load('module', $dependency);
- $modules[$dependency] = 0;
+ // Initialize the localization system.
+ // @todo We actually need to query the database whether the site has any
+ // localization module enabled, and load it automatically.
+ $locale = drupal_init_language();
+ // Invoke implementations of hook_init().
+ module_invoke_all('init');
}
- }
- // Reset module list.
- module_list(FALSE, TRUE, FALSE, $modules);
+ // Invoke callback function.
+ return call_user_func_array($info['callback'], $args);
+ break; // phase 2
+ } // switch $phase
+}
- // Initialize the localization system.
- // @todo We actually need to query the database whether the site has any
- // localization module enabled, and load it automatically.
- $locale = drupal_init_language();
- // Invoke implementations of hook_init().
- module_invoke_all('init');
- }
+// run first phase to find out whether xss filter function is missing
+if (!js_execute_callback(1)) {
- // Invoke callback function.
- return call_user_func_array($info['callback'], $args);
-}
+ /**
+ * l() calls check_url(), which needs to check for XSS attacks.
+ */
+ function filter_xss_bad_protocol($string, $decode = TRUE) {
+ static $allowed_protocols;
+ if (!isset($allowed_protocols)) {
+ $allowed_protocols = array_flip(variable_get('filter_allowed_protocols', array('http', 'https', 'ftp', 'news', 'nntp', 'telnet', 'mailto', 'irc', 'ssh', 'sftp', 'webcal')));
+ }
-/**
- * l() calls check_url(), which needs to check for XSS attacks.
- */
-function filter_xss_bad_protocol($string, $decode = TRUE) {
- static $allowed_protocols;
- if (!isset($allowed_protocols)) {
- $allowed_protocols = array_flip(variable_get('filter_allowed_protocols', array('http', 'https', 'ftp', 'news', 'nntp', 'telnet', 'mailto', 'irc', 'ssh', 'sftp', 'webcal')));
- }
+ // Get the plain text representation of the attribute value (i.e. its meaning).
+ if ($decode) {
+ $string = decode_entities($string);
+ }
- // Get the plain text representation of the attribute value (i.e. its meaning).
- if ($decode) {
- $string = decode_entities($string);
- }
+ // Iteratively remove any invalid protocol found.
- // Iteratively remove any invalid protocol found.
-
- do {
- $before = $string;
- $colonpos = strpos($string, ':');
- if ($colonpos > 0) {
- // We found a colon, possibly a protocol. Verify.
- $protocol = substr($string, 0, $colonpos);
- // If a colon is preceded by a slash, question mark or hash, it cannot
- // possibly be part of the URL scheme. This must be a relative URL,
- // which inherits the (safe) protocol of the base document.
- if (preg_match('![/?#]!', $protocol)) {
- break;
- }
- // Per RFC2616, section 3.2.3 (URI Comparison) scheme comparison must be case-insensitive.
- // Check if this is a disallowed protocol.
- if (!isset($allowed_protocols[strtolower($protocol)])) {
- $string = substr($string, $colonpos + 1);
- }
+ do {
+ $before = $string;
+ $colonpos = strpos($string, ':');
+ if ($colonpos > 0) {
+ // We found a colon, possibly a protocol. Verify.
+ $protocol = substr($string, 0, $colonpos);
+ // If a colon is preceded by a slash, question mark or hash, it cannot
+ // possibly be part of the URL scheme. This must be a relative URL,
+ // which inherits the (safe) protocol of the base document.
+ if (preg_match('![/?#]!', $protocol)) {
+ break;
+ }
+ // Per RFC2616, section 3.2.3 (URI Comparison) scheme comparison must be case-insensitive.
+ // Check if this is a disallowed protocol.
+ if (!isset($allowed_protocols[strtolower($protocol)])) {
+ $string = substr($string, $colonpos + 1);
+ }
+ }
+ } while ($before != $string);
+ return check_plain($string);
}
- } while ($before != $string);
- return check_plain($string);
}
-$return = js_execute_callback();
+// continue with phase 2 to engage modules
+$return = js_execute_callback(2);
// Menu status constants are integers; page content is a string.
if (is_int($return)) {PS. If I was wrong at some point, pls let me know. (If you found it helpful: Too. :-))
| Attachment | Size |
|---|---|
| patch.diff | 9.48 KB |
Comments
#1
(also forgotten to be set to correct status)
#2
Hi, I just revised my fixes as to current coding standards and also a bit to restore some of your initial logic the first patch destroyed.
Find attached the latest patch and also the entire js.php file in my extended version. Hopefully you will use it.