From 48050aca410a845087b7d43589180aa7a7130944 Mon Sep 17 00:00:00 2001 From: Andy Staudacher Date: Mon, 31 Aug 2009 19:53:53 -0700 Subject: Add XSS check to ensure that html::js_string() is not preceded by a quote. --- modules/gallery/tests/Xss_Security_Test.php | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-) (limited to 'modules') diff --git a/modules/gallery/tests/Xss_Security_Test.php b/modules/gallery/tests/Xss_Security_Test.php index 1d1acce8..05fc052a 100644 --- a/modules/gallery/tests/Xss_Security_Test.php +++ b/modules/gallery/tests/Xss_Security_Test.php @@ -35,6 +35,7 @@ class Xss_Security_Test extends Unit_Test_Case { $inline_html = ""; $in_attribute_js_context = false; $href_attribute_start = false; + $preceded_by_quote = false; for ($token_number = 0; $token_number < count($tokens); $token_number++) { $token = $tokens[$token_number]; @@ -88,6 +89,8 @@ class Xss_Security_Test extends Unit_Test_Case { $href_attribute_start = preg_match('{\bhref\s*=\s*[\'"]?\s*$}i', $inline_html); + $preceded_by_quote = preg_match('{[\'"]\s*$}i', $inline_html); + $pos = false; if ($in_attribute_js_context && ($pos = strpos($inline_html, $delimiter)) !== false) { $in_attribute_js_context = false; @@ -113,7 +116,8 @@ class Xss_Security_Test extends Unit_Test_Case { } else if ($token[0] == T_OPEN_TAG_WITH_ECHO) { // No need for a stack here - assume < ? = cannot be nested. $frame = self::_create_frame($token, $in_script_block, - $href_attribute_start, $in_attribute_js_context); + $href_attribute_start, $in_attribute_js_context, + $preceded_by_quote); $href_attribute_start = false; } else if ($frame && $token[0] == T_CLOSE_TAG) { // Store the < ? = ... ? > block that just ended here. @@ -290,7 +294,7 @@ class Xss_Security_Test extends Unit_Test_Case { $state = "ILLEGAL"; } else if ($frame->in_script_block()) { $state = "DIRTY_JS"; - if ($frame->is_safe_js()) { + if ($frame->is_safe_js() && !$frame->preceded_by_quote()) { $state = "CLEAN"; } } else if ($frame->in_attribute_js_context()) { @@ -327,9 +331,11 @@ class Xss_Security_Test extends Unit_Test_Case { } private static function _create_frame($token, $in_script_block, - $href_attribute_start, $in_attribute_js_context) { + $href_attribute_start, $in_attribute_js_context, + $preceded_by_quote) { return new Xss_Security_Test_Frame($token[2], $in_script_block, - $href_attribute_start, $in_attribute_js_context); + $href_attribute_start, $in_attribute_js_context, + $preceded_by_quote); } private static function _token_matches($expected_token, &$tokens, $token_number) { @@ -360,14 +366,17 @@ class Xss_Security_Test_Frame { private $_in_href_attribute = false; private $_is_safe_href_attr = false; private $_in_attribute_js_context = false; + private $_preceded_by_quote; private $_line; function __construct($line_number, $in_script_block, - $href_attribute_start, $in_attribute_js_context) { + $href_attribute_start, $in_attribute_js_context, + $preceded_by_quote) { $this->_line = $line_number; $this->_in_script_block = $in_script_block; $this->_in_href_attribute = $href_attribute_start; $this->_in_attribute_js_context = $in_attribute_js_context; + $this->_preceded_by_quote = $preceded_by_quote; } function expr() { @@ -411,6 +420,10 @@ class Xss_Security_Test_Frame { return $this->_is_safe_js; } + function preceded_by_quote() { + return $this->_preceded_by_quote; + } + function line() { return $this->_line; } -- cgit v1.2.3 From 0513713fde119bd50978237506cd6e21df001e61 Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Mon, 31 Aug 2009 21:05:21 -0700 Subject: Add 'organize album' to the context menu. --- modules/organize/helpers/organize_event.php | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) (limited to 'modules') diff --git a/modules/organize/helpers/organize_event.php b/modules/organize/helpers/organize_event.php index 7d6b3e24..79077db3 100644 --- a/modules/organize/helpers/organize_event.php +++ b/modules/organize/helpers/organize_event.php @@ -21,7 +21,7 @@ class organize_event_Core { static function site_menu($menu, $theme) { $item = $theme->item(); - if ($item && access::can("edit", $item) && $item->is_album()) { + if ($item && $item->is_album() && access::can("edit", $item)) { $menu->get("options_menu") ->append(Menu::factory("dialog") ->id("organize") @@ -30,4 +30,16 @@ class organize_event_Core { ->url(url::site("organize/dialog/{$item->id}"))); } } + + static function context_menu($menu, $theme, $item) { + if ($item->is_album() && access::can("edit", $item)) { + $menu->get("options_menu") + ->append(Menu::factory("dialog") + ->id("organize") + ->label(t("Organize album")) + ->css_id("gOrganizeLink") + ->url(url::site("organize/dialog/{$item->id}"))); + } + } + } -- cgit v1.2.3 From c8871705550914f295f887d4a05bdec6a42a8d9e Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Mon, 31 Aug 2009 21:10:22 -0700 Subject: Stay on the same page when editing albums/movies/photos. Fixes ticket --- modules/gallery/controllers/albums.php | 3 +-- modules/gallery/controllers/movies.php | 3 +-- modules/gallery/controllers/photos.php | 3 +-- 3 files changed, 3 insertions(+), 6 deletions(-) (limited to 'modules') diff --git a/modules/gallery/controllers/albums.php b/modules/gallery/controllers/albums.php index 78f12c80..8ceff0f9 100644 --- a/modules/gallery/controllers/albums.php +++ b/modules/gallery/controllers/albums.php @@ -197,8 +197,7 @@ array("album_title" => html::purify($album->title)))); print json_encode( - array("result" => "success", - "location" => url::site("albums/$album->id"))); + array("result" => "success")); } else { print json_encode( array("result" => "error", diff --git a/modules/gallery/controllers/movies.php b/modules/gallery/controllers/movies.php index 09b16759..c40cde9e 100644 --- a/modules/gallery/controllers/movies.php +++ b/modules/gallery/controllers/movies.php @@ -96,8 +96,7 @@ class Movies_Controller extends Items_Controller { t("Saved photo %photo_title", array("photo_title" => $photo->title))); print json_encode( - array("result" => "success", - "location" => url::site("photos/$photo->id"))); + array("result" => "success")); } else { print json_encode( array("result" => "error", diff --git a/modules/gallery/controllers/photos.php b/modules/gallery/controllers/photos.php index 3b9662c7..dd6d3ab5 100644 --- a/modules/gallery/controllers/photos.php +++ b/modules/gallery/controllers/photos.php @@ -90,8 +90,7 @@ class Photos_Controller extends Items_Controller { array("photo_title" => html::purify($photo->title)))); print json_encode( - array("result" => "success", - "location" => url::site("photos/$photo->id"))); + array("result" => "success")); } else { print json_encode( array("result" => "error", -- cgit v1.2.3 From 50c8b964053d0b16fe842ff70f5b6c8e27c1a757 Mon Sep 17 00:00:00 2001 From: Andy Staudacher Date: Mon, 31 Aug 2009 21:17:35 -0700 Subject: Add XSS check for HTML attributes --- modules/gallery/tests/Xss_Security_Test.php | 53 ++++++++++++++++++++++++----- 1 file changed, 44 insertions(+), 9 deletions(-) (limited to 'modules') diff --git a/modules/gallery/tests/Xss_Security_Test.php b/modules/gallery/tests/Xss_Security_Test.php index 05fc052a..7a6589bd 100644 --- a/modules/gallery/tests/Xss_Security_Test.php +++ b/modules/gallery/tests/Xss_Security_Test.php @@ -34,6 +34,7 @@ class Xss_Security_Test extends Unit_Test_Case { $in_script_block = false; $inline_html = ""; $in_attribute_js_context = false; + $in_attribute = false; $href_attribute_start = false; $preceded_by_quote = false; @@ -87,23 +88,31 @@ class Xss_Security_Test extends Unit_Test_Case { } } - $href_attribute_start = preg_match('{\bhref\s*=\s*[\'"]?\s*$}i', $inline_html); - $preceded_by_quote = preg_match('{[\'"]\s*$}i', $inline_html); $pos = false; - if ($in_attribute_js_context && ($pos = strpos($inline_html, $delimiter)) !== false) { + if (($in_attribute || $in_attribute_js_context) && + ($pos = strpos($inline_html, $delimiter)) !== false) { $in_attribute_js_context = false; + $in_attribute = false; + $href_attribute_start = false; } - if (!$in_attribute_js_context) { + if (!$in_attribute_js_context || !$in_attribute) { $pos = ($pos === false) ? 0 : $pos; if (preg_match('{\bhref\s*=\s*(")javascript:[^"]*$}i', $inline_html, $matches, 0, $pos) || preg_match("{\bhref\s*=\s*(')javascript:[^']*$}i", $inline_html, $matches, 0, $pos) || preg_match("{\bon[a-z]+\s*=\s*(')[^']*$}i", $inline_html, $matches, 0, $pos) || preg_match('{\bon[a-z]+\s*=\s*(")[^"]*$}i', $inline_html, $matches, 0, $pos)) { $in_attribute_js_context = true; + $in_attribute = true; $delimiter = $matches[1]; $inline_html = ""; + } else if (preg_match('{\b([a-z]+)\s*=\s*(")([^"]*)$}i', $inline_html, $matches, 0, $pos) || + preg_match("{\b([a-z]+)\s*=\s*(')([^']*)$}i", $inline_html, $matches, 0, $pos)) { + $in_attribute = true; + $delimiter = $matches[2]; + $inline_html = ""; + $href_attribute_start = strtolower($matches[1]) == "href" && empty($matches[3]); } } @@ -117,7 +126,7 @@ class Xss_Security_Test extends Unit_Test_Case { // No need for a stack here - assume < ? = cannot be nested. $frame = self::_create_frame($token, $in_script_block, $href_attribute_start, $in_attribute_js_context, - $preceded_by_quote); + $in_attribute, $preceded_by_quote); $href_attribute_start = false; } else if ($frame && $token[0] == T_CLOSE_TAG) { // Store the < ? = ... ? > block that just ended here. @@ -207,6 +216,7 @@ class Xss_Security_Test extends Unit_Test_Case { self::_token_matches("(", $tokens, $token_number + 3)) { $frame->is_safe_html(true); $frame->is_safe_href_attr(true); + $frame->is_safe_attr(true); $method = $tokens[$token_number + 2][1]; $frame->expr_append("::$method("); @@ -233,6 +243,9 @@ class Xss_Security_Test extends Unit_Test_Case { } else { $frame->is_safe_html(true); } + if ("clean_attribute" == $method) { + $frame->is_safe_attr(true); + } } } } else if ($frame && $token[0] == T_OBJECT_OPERATOR) { @@ -253,6 +266,9 @@ class Xss_Security_Test extends Unit_Test_Case { } else { $frame->is_safe_html(true); } + if ("for_html_attr" == $method) { + $frame->is_safe_attr(true); + } } } else if ($frame) { $frame->expr_append($token[1]); @@ -305,6 +321,11 @@ class Xss_Security_Test extends Unit_Test_Case { if ($frame->is_safe_href_attr()) { $state = "CLEAN"; } + } else if ($frame->in_attribute()) { + $state = "DIRTY_ATTR"; + if ($frame->is_safe_attr()) { + $state = "CLEAN"; + } } else { if ($frame->is_safe_html()) { $state = "CLEAN"; @@ -332,10 +353,10 @@ class Xss_Security_Test extends Unit_Test_Case { private static function _create_frame($token, $in_script_block, $href_attribute_start, $in_attribute_js_context, - $preceded_by_quote) { + $in_attribute, $preceded_by_quote) { return new Xss_Security_Test_Frame($token[2], $in_script_block, $href_attribute_start, $in_attribute_js_context, - $preceded_by_quote); + $in_attribute, $preceded_by_quote); } private static function _token_matches($expected_token, &$tokens, $token_number) { @@ -366,16 +387,19 @@ class Xss_Security_Test_Frame { private $_in_href_attribute = false; private $_is_safe_href_attr = false; private $_in_attribute_js_context = false; - private $_preceded_by_quote; + private $_in_attribute = false; + private $_preceded_by_quote = false; + private $_is_safe_attr = false; private $_line; function __construct($line_number, $in_script_block, $href_attribute_start, $in_attribute_js_context, - $preceded_by_quote) { + $in_attribute, $preceded_by_quote) { $this->_line = $line_number; $this->_in_script_block = $in_script_block; $this->_in_href_attribute = $href_attribute_start; $this->_in_attribute_js_context = $in_attribute_js_context; + $this->_in_attribute = $in_attribute; $this->_preceded_by_quote = $preceded_by_quote; } @@ -395,6 +419,10 @@ class Xss_Security_Test_Frame { return $this->_in_href_attribute; } + function in_attribute() { + return $this->_in_attribute; + } + function in_attribute_js_context() { return $this->_in_attribute_js_context; } @@ -413,6 +441,13 @@ class Xss_Security_Test_Frame { return $this->_is_safe_href_attr; } + function is_safe_attr($new_val=NULL) { + if ($new_val !== NULL) { + $this->_is_safe_attr = (bool) $new_val; + } + return $this->_is_safe_attr; + } + function is_safe_js($new_val=NULL) { if ($new_val !== NULL) { $this->_is_safe_js = (bool) $new_val; -- cgit v1.2.3 From 8c3a2db3803ccaa3572f0bf061ca7faf62f13fca Mon Sep 17 00:00:00 2001 From: Andy Staudacher Date: Mon, 31 Aug 2009 21:28:37 -0700 Subject: Fix typo in description --- modules/rss/module.info | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'modules') diff --git a/modules/rss/module.info b/modules/rss/module.info index 81ee7848..48375da1 100644 --- a/modules/rss/module.info +++ b/modules/rss/module.info @@ -1,3 +1,3 @@ name = "RSS" -description = "Provide a RSS feeds" +description = "Provides RSS feeds" version = 1 -- cgit v1.2.3 From 2bc73e2e36fefc3c1ee1b8e97e686c6729e58dcb Mon Sep 17 00:00:00 2001 From: Andy Staudacher Date: Mon, 31 Aug 2009 21:51:57 -0700 Subject: Fix XSS vectors in HTML attributes (mostly t() calls) --- .../views/admin_block_recent_comments.html.php | 2 +- modules/comment/views/admin_comments.html.php | 2 +- modules/comment/views/comment.html.php | 2 +- modules/comment/views/comments.html.php | 2 +- modules/exif/views/exif_sidebar.html.php | 2 +- .../gallery/views/admin_advanced_settings.html.php | 2 +- .../views/admin_block_photo_stream.html.php | 4 +-- modules/gallery/views/admin_modules.html.php | 2 +- modules/gallery/views/admin_themes.html.php | 12 +++---- modules/gallery/views/after_install.html.php | 2 +- .../gallery/views/after_install_loader.html.php | 2 +- modules/gallery/views/l10n_client.html.php | 2 +- modules/gallery/views/move_browse.html.php | 2 +- modules/gallery/views/permissions_form.html.php | 42 +++++++++++----------- modules/gallery/views/simple_uploader.html.php | 2 +- modules/rss/views/feed.mrss.php | 10 +++--- modules/search/views/search_link.html.php | 2 +- modules/user/views/admin_users.html.php | 10 +++--- modules/user/views/admin_users_group.html.php | 6 ++-- modules/user/views/login.html.php | 2 +- modules/watermark/views/admin_watermarks.html.php | 6 ++-- themes/admin_default/views/admin.html.php | 2 +- themes/default/views/page.html.php | 4 +-- themes/default/views/photo.html.php | 2 +- 24 files changed, 63 insertions(+), 63 deletions(-) (limited to 'modules') diff --git a/modules/comment/views/admin_block_recent_comments.html.php b/modules/comment/views/admin_block_recent_comments.html.php index dc3975e0..2afa5bf8 100644 --- a/modules/comment/views/admin_block_recent_comments.html.php +++ b/modules/comment/views/admin_block_recent_comments.html.php @@ -4,7 +4,7 @@
  • "> " class="gAvatar" - alt="author_name()) ?>" + alt="author_name()) ?>" width="32" height="32" /> created) ?> diff --git a/modules/comment/views/admin_comments.html.php b/modules/comment/views/admin_comments.html.php index 588c3ebc..f5970ae1 100644 --- a/modules/comment/views/admin_comments.html.php +++ b/modules/comment/views/admin_comments.html.php @@ -122,7 +122,7 @@ has_thumb()): ?> <?= html::purify($item->title) ?>thumb_width, $item->thumb_height, 75) ?> /> diff --git a/modules/comment/views/comment.html.php b/modules/comment/views/comment.html.php index 1d0786cb..ce4e197d 100644 --- a/modules/comment/views/comment.html.php +++ b/modules/comment/views/comment.html.php @@ -4,7 +4,7 @@ " class="gAvatar" - alt="author_name()) ?>" + alt="author_name()) ?>" width="40" height="40" /> diff --git a/modules/comment/views/comments.html.php b/modules/comment/views/comments.html.php index 1e45c946..b7ebdf3a 100644 --- a/modules/comment/views/comments.html.php +++ b/modules/comment/views/comments.html.php @@ -18,7 +18,7 @@ " class="gAvatar" - alt="author_name()) ?>" + alt="author_name()) ?>" width="40" height="40" /> diff --git a/modules/exif/views/exif_sidebar.html.php b/modules/exif/views/exif_sidebar.html.php index ee528613..60c0e1d4 100644 --- a/modules/exif/views/exif_sidebar.html.php +++ b/modules/exif/views/exif_sidebar.html.php @@ -1,5 +1,5 @@ -id}") ?>" title="" +id}") ?>" title="for_html_attr() ?>" class="gDialogLink gButtonLink ui-icon-left ui-state-default ui-corner-all"> diff --git a/modules/gallery/views/admin_advanced_settings.html.php b/modules/gallery/views/admin_advanced_settings.html.php index 4235e8f8..c3595da5 100644 --- a/modules/gallery/views/admin_advanced_settings.html.php +++ b/modules/gallery/views/admin_advanced_settings.html.php @@ -24,7 +24,7 @@ module_name/" . html::clean($var->name)) ?>" class="gDialogLink" - title=" $var->name, "module_name" => $var->module_name)) ?>"> + title=" $var->name, "module_name" => $var->module_name))->for_html_attr() ?>"> value): ?> value) ?> diff --git a/modules/gallery/views/admin_block_photo_stream.html.php b/modules/gallery/views/admin_block_photo_stream.html.php index a50836ad..1b9d8ff5 100644 --- a/modules/gallery/views/admin_block_photo_stream.html.php +++ b/modules/gallery/views/admin_block_photo_stream.html.php @@ -2,9 +2,9 @@
    • - id") ?>" title="title) ?>"> + id") ?>" title="title)->for_html_attr() ?>"> width, $photo->height, 72) ?> - src="thumb_url() ?>" alt="title) ?>" /> + src="thumb_url() ?>" alt="title)->for_html_attr() ?>" />
    • diff --git a/modules/gallery/views/admin_modules.html.php b/modules/gallery/views/admin_modules.html.php index 168e20d0..9cf03cb3 100644 --- a/modules/gallery/views/admin_modules.html.php +++ b/modules/gallery/views/admin_modules.html.php @@ -27,6 +27,6 @@ - "/> + for_html_attr() ?>"/> diff --git a/modules/gallery/views/admin_themes.html.php b/modules/gallery/views/admin_themes.html.php index dc13a6a0..0aac4717 100644 --- a/modules/gallery/views/admin_themes.html.php +++ b/modules/gallery/views/admin_themes.html.php @@ -16,7 +16,7 @@

      " - alt="name ?>" /> + alt="name) ?>" />

      name ?>

      description ?> @@ -30,9 +30,9 @@ site) continue ?>

      - " class="gDialogLink" title=" $info->name)) ?>"> + " class="gDialogLink" title=" $info->name))->for_html_attr() ?>"> " - alt="name ?>" /> + alt="name) ?>" />

      name ?>

      description ?> @@ -54,7 +54,7 @@

      " - alt="name ?>" /> + alt="name) ?>" />

      name ?>

      description ?> @@ -68,9 +68,9 @@ admin) continue ?>

      - " class="gDialogLink" title=" $info->name)) ?>"> + " class="gDialogLink" title=" $info->name))->for_html_attr() ?>"> " - alt="name ?>" /> + alt="name) ?>" />

      name ?>

      description ?> diff --git a/modules/gallery/views/after_install.html.php b/modules/gallery/views/after_install.html.php index b77a1707..897946a2 100644 --- a/modules/gallery/views/after_install.html.php +++ b/modules/gallery/views/after_install.html.php @@ -13,7 +13,7 @@

      id}") ?>" - title="" + title="for_html_attr() ?>" id="gAfterInstallChangePasswordLink" class="gButtonLink ui-state-default ui-corners-all">