21 KiB

Raw Blame History

2.4.3 (2010-11-11)

Overview

Fixed a security issue where destructive controller actions are not correctly secured against Cross-Site Request Forgery (CSRF). This affects various CMS interfaces, as well as classes based on TableListField or ComplexTableField.
Enhance the protection of the assets/ directory in both IIS and Apache by including a file type whitelist.
Compatibility with PHPUnit 3.5
Allow direct test execution through the "phpunit" binary, in addition to the existing "sake" executable and TestRunner class.
Misc. fixes to validation in date-based form fields, as well as better formatting of dates in non-default locales.

Upgrading Notes

Important: If you are running PHP as FastCGI with Apache

Your development environment, or production web host may be running PHP as FastCGI with Apache. If this is the case, there is a regression in 2.4.3 which will break your site. There are two ways to resolve this problem:

Don't upgrade to 2.4.3 for now, until 2.4.4 is released
Patch the assets/.htaccess file like this: http://open.silverstripe.org/changeset/113809

This does NOT affect IIS, or other web servers that don't understand .htaccess files.

If you're not sure whether PHP is running as FastCGI in your server environment, please check the output of phpinfo(). The easiest way to do this is to open mysite/_config.php and add phpinfo() to the bottom of the file, then browse to your site to see the environment information.

Forum references of where the community have had issues:

Important: Add manual request forgery protection to destructive controller actions

Cross Site Request Forgery (CSRF) allows an attacker to initiate unauthorized actions against a victim with a valid login to a target site in the same browser session, e.g. a login to SilverStripe CMS (read more about CSRF).

While SilverStripe protects form submissions from CSRF automatically, destructive GET and POST actions in subclasses of Controller are vulnerable without manual checks.

You will need to review any custom subclasses of RequestHandler and Controller (including subclasses of FormField), and add manual checks. Best practice is to return a "400 Bad Request" HTTP error when the CSRF check fails.

Also review the method signatures of any form actions - these are only protected automatically with the correct count of parameters (//$data, $form// instead of $request).

:::php
// Form field actions
class MyFormField extends FormField {

  // Form fields always have a reference to their form. 
  // Use the form-specific token instance.
  function delete($request) {
    $token = $this->getForm()->getSecurityToken();
    if(!$token->checkRequest($request)) return $this->httpError(400);
  
    // valid form field delete action
  }
}
  
// Controller actions (GET and POST) without form
class MyController extends Controller {

  // Manually adds token to link
  function DeleteLink() {
    $token = SecurityToken::inst();
    $link = Controller::join_links($this->Link(), 'delete');
    // will add "?SecurityID=`<random value>`"
    $link = $token->addToUrl($link); 
    return $link;
  }

  // Controller actions pass through the request object,
  // not called through a form.
  // Use a global token instance.
  function delete(SS_HTTPRequest $request) {
    $token = SecurityToken::inst();
    if(!$token->checkRequest($request)) return $this->httpError(400);
  
    // valid controller delete action
  }
}

// Controller actions (GET and POST) with form
class MyController extends Controller {
  
  // Forms have CSRF protection turned on by default,
  // will add a HiddenField instance called "SecurityID"
  function Form() {
    return new Form(
      $this, 'Form', new FieldSet(), new FieldSet(new FormAction('submit'))
    );
  }
  
  // Form->httpSubmission() checks for CSRF automatically,
  // but you need to include both parameters in the method signature.
  function submit($data, Form $form) {
    // valid submit action
  }
  
}

Note: It is regarded as good practice in HTTP to only have destructive actions in POST submissions rather than GET links.

If you have overwritten any CMS templates (based on LeftAndMain), you will need to update them to include "SecurityID" parameter in all manually created forms.

Affected classes and methods:

AssetAdmin->addfolder()
AssetAdmin->deletefolder()
AssetAdmin->deleteunusedthumbnails()
AssetAdmin->removefile()
AssetTableField
CMSMain->addpage()
CMSMain->buildbrokenlinks()
CMSMain->createtranslation()
CMSMain->duplicate()
CMSMain->duplicatewithchildren()
CMSMain->publishall()
CMSMain->restore()
CMSMain->rollback()
CMSMain->unpublish()
CommentTableField
ComplexTableField
LeftAndMain->ajaxupdateparent()
LeftAndMain->ajaxupdatesort()
LeftAndMain->deleteitems()
MemberTableField
MemberTableField->addtogroup()
MemberTableField->delete()
MemberTableField_ItemRequest->delete()
PageComment
PageComment_Controller
SecurityAdmin->addgroup()
SecurityAdmin->addmember()
SecurityAdmin->MemberForm()
SecurityAdmin->removememberfromgroup()
SecurityAdmin->savemember()
TableListField

Usage of Controller::join_links() to concatenate links now mandatory

The [api:Controller::join_links()] method to create links within SilverStripe controllers is now mandatory. This method ensures that links with existing GET parameters don't break through string concatenation.

:::php
// bad
$link = $this->Link() . 'export?csv=1';

// good
$link = Controller::join_links($this->Link(), 'export', '?csv=1');

Full controller example:

:::php
class MyController extends Controller {

	function export($request) {
		// ...
	}
	
	function Link($action = null) {
		return Controller::join_links('MyController', $action);
	}

	function ExportLink() {
		return Controller::join_links($this->Link('export'), '?csv=1');
	}
}

Using this method is particularly important for any custom [api:TableListField] or [api:ComplexTableField] subclasses and any [api:LeftAndMain] subclass for the CMS UI. These classes in particular were refactored to secure destructive links against Cross Site Request Forgery (CSRF). This is achieved via a mandatory "SecurityID" GET parameter appended to the base link.

Auto-disabled "SecurityID" field in FunctionalTest

"SecurityID" tokens are now disabled by default in unit tests, to make form submissions easier. You can manually enable security tokens, either globally or for a specific form.

:::php
class MyTest extends SapphireTest {

	// option 1: enable for all forms created through this test
	function setUp() {
		parent::setUp();
		SecurityToken::enable();
	}
	
	// option 2: enable for one specific form
	function testMyForm() {
		$form = new MyForm();
		$form->enableSecurityToken();
	}
}

2.4.3 Changelog

Features and Enhancements

[rev:113420] Validation for uploaded files
[rev:113284] Added Form->enableSecurityToken() as a counterpart to the existing disableSecurityToken()
[rev:113272] Added !SecurityToken to wrap CSRF protection via "SecurityID" request parameter
[rev:112272] MySQLDatabase::renameField() no longer checks that the field exists in fieldList(). alterField() does no such check, so it should be consistent. Removing this should provide a small performance improvement as well
[rev:111915] Added localisation for batch actions in javascript + translations
[rev:111891] #4903 !MemberLoginForm field for "You are logged in as %s" message customisation (thanks walec51!)
[rev:111887] #3775 Added getter to GD so you can retrieve the internal GD resource being used. Made setGD public so you can override the GD yourself as well
[rev:111873] Show "Database Configuration" section of installer requirements for reference (collapsed by default)
[rev:111868] MySQLDatabase::getVersion() now uses mysql_get_server_info() which has been supported since PHP 4. This gives us a better version than say "5.1", instead we now get something like "5.1.51"
[rev:111850] Make use of mysql_get_server_info() when calling MSSQLDatabase::getVersion(), if there's a problem getting info this way, falls back to using query for VERSION() details
[rev:111828] 6017 - Configurable help link
[rev:111495] Making "sake" script more portable by using "/usr/bin/env" shebang instead of "/bin/bash" (fixes #6045, thanks sychan)
[rev:111489] Added "module=" argument to !FullTestSuite (to support comma-separated module lists)
[rev:111449] allow !PageCommentForm to store all users data, rather than hardcoding the fields
[rev:111443] simple extend hook for !PageCommentForms. Temporary measure till #6053 is implemented
[rev:111086] #6023 Shorten SSViewer cached template path for readability of the filenames, and also so Windows doesn't break on long paths
[rev:111050] Added custom test listener for PHPUnit in order to call setUpOnce() and tearDownOnce() on !SapphireTest
[rev:111048] Allowing to run single tests via phpunit through new test bootstrap XML file (e.g. "phpunit sapphire/tests/api/!RestfulServerTest.php" or "phpunit sapphire/tests/api")
[rev:111045] Added !FullTestSuite.php, so that you can test by running "phpunit sapphire/tests/!FullTestSuite".
[rev:111041] refactored runTests, using the new phpunit wrapper classes.
[rev:111039] Created a phpunit wrapper class to ensure that Sapphire's test framework is capable of running unit tests, coverage report and retrieve clover-statistics for PHPUnit 3.4 and PHPUnit 3.5

API Changes

[rev:113282] Fixed various controllers to enforce CSRF protection through Form_!SecurityToken on GET actions that are not routed through Form->httpSubmission(): !AssetAdmin, CMSBatchActionHandler, CMSMain, !CommentTableField, !LeftAndMain, !MemberTableField, !PageComment, !PageComment_Controller
[rev:113275] Added security token to !TableListField->Link() in order to include it in all URL actions automatically. This ensures that field actions bypassing Form->httpSubmission() still get CSRF protection

Bugfixes

[rev:113590] ErrorPage::requireDefaultRecords() case where no assets directory causes an fopen() error. Ensure assets directory is created before attempting to write error page files
[rev:113419] Better checking of file validity (#6093) Thanks Pigeon
[rev:113295] Ensure that !SearchForm searchEngine() call properly escapes the Relevance field for ANSI compliance
[rev:113277] Clear static marking caches on Hierarchy->flushCache()
[rev:113276] Fixed !ComplexTableField and !TableListField GET actions against CSRF attacks (with Form_!SecurityToken->checkRequest())
[rev:113273] Using current controller for !MemberTableField constructor in Group->getCMSFields() instead of passing in a wrong instance (Group)
[rev:113249] ModelViewer doesn't work due to minor bug introduced by making $_CLASS_MANIFEST keys lowercase (fixes #6144, thanks daniel.lindkvist)
[rev:113247] Fixed month conversion in !DateField_View_JQuery::convert_iso_to_jquery_format() (fixes #6124, thanks mbren and natmchugh)
[rev:113193] removed taiwans province of china
[rev:113157] Add PHPUnit includes to !SapphireTest? class (can be loaded outside of !TestRunner? for static calls, in which case the PHPUnit autoloaders/includes aren't in place yet) (merged from r113156)
[rev:113107] Use correct language code for jquery-ui date picker for en_US
[rev:112961] Don't include web.config in the assets tracked in the File table.
[rev:112288] Renamed !MySQLQuery::__destroy() renamed to __destruct() so that it is called properly after the object is destroyed
[rev:112258] one more requirement switched to SSL
[rev:111949] Ensure that \r carriage return characters get stripped out before setting content in HTMLValue::setContent(). DOMDocument will transform these into &#13 entities, which is apparently XML spec, but not necessary for us as we're using HTML
[rev:111932] #6089 Avoid javascript error when "Allow drag & drop reordering" enabled, and attempt to drag a file from one folder to another is performed
[rev:111914] #6096 RSSFeed::feedContent() restores previous state of SSViewer::get_source_file_comments() after temporarily disabling it (thanks paradigmincarnate!)
[rev:111898] Filesystem::removeFolder() did not remove files that ended with a "." when this is a valid file. Remove the regex and replace with specific case for "." and ".."
[rev:111890] #6066 Form::__construct() should respect hasMethod on passed in Controller instance if it's available (thanks paradigmincarnate!)
[rev:111889] #3910 Setting timezone parameter to !MySQLDatabase::__construct() should use $this->query() to be consistent
[rev:111878] Ensure that windows-style newlines ("\r\n") don't get converted to their XML entity representation through DOMDocument in SS_HTMLValue->setContent()
[rev:111843] More common defaults for en_US.xml used by Zend_!DateFormat (and !DateField/!DatetimeField), with less error prone numerical format replacing the Zend default of shortened month names (fixes #6071, thanks dalesaurus)
[rev:111843] Correct locale mapping in !DateField_View_JQuery for "en_US" and "en_NZ"
[rev:111842] #6055 !ErrorPage should always create static error page files when dev/build is called if they don't exist
[rev:111841] RFC 2822 compliant validation of email adresses in !EmailField->jsValidation() and !EmailField->validate() (fixes #6067, thanks paradigmincarnate)
[rev:111772] DB::connect() should not rely on $_SESSION existing, so we check isset() to supress any warnings of undefined indexes
[rev:111494] Changing File->Filename property from arbitrary limitation on VARCHAR (255 characters) to TEXT (65k characters) to ensure the framework can handle deeply nested filesystem trees (fixes #6015, thanks muzdowski)
[rev:111493] Moving folder after executing Folder::findOrMake will not set the Filenames properly. Invoking updateFilesystem() in File->onAfterWrite() instead of onBeforeWrite(), and avoid caching in FIle->getRelativePath() (fixes #5994 and #5937, thanks muzdowski)
[rev:111492] Removing overloaded !TableField->sourceItems() method, which enables features of the underlying !TableListField implementation, such as pagination and source item caching (fixed #5965, thanks martijn)
[rev:111464] Search didn't respect searchableClasses passed to !FulltextSearchable::enable()
[rev:111452] added validation to the page comment form
[rev:111255] ContentController::!SiteConfig() should look to the !SiteTree record so an alternate !SiteConfig is considered, if this method doesn't exist on the data record then fall back to the default !SiteConfig
[rev:111202] Fixed quoting and GROUP BY statement in !ManyManyComplexTableField->getQuery() for Postgres compatibility
[rev:111176] Force tidy to avoid wrapping long lines in CSSContentParser, it breaks our !FunctionalTest string assertions
[rev:111126] TarballArchive::extractTo() uses an incorrectly spelled argument
[rev:111097] Fixed !PhpSyntaxTest not to rely on relative folder references (broken due to chdir() changes in cli-script.php and bootstrap.php)
[rev:111092] Fixed regression where coverage report request did not get passed through to runTests() in !TestRunner::all()
[rev:111091] Fixed regression of dev/tests/all running a coverage report instead of just unit tests
[rev:111049] Unset $default_session when using Session::clear_all()
[rev:111044] Allow execution of a test without a current controller.
[rev:111043] Don't require a current controller for Session::get/set/etc to work.

Minor changes

[rev:113450] Fixed output spelling mistake and formatting in !SapphireTest::delete_all_temp_dbs()
[rev:113430] Fixed RSSFeedTest which should put test configuration code into setUp() and tearDown() methods. If the test fails halfway through, these will get called to clean up the state
[rev:113360] Fixed regression from r113282 for changed !SecurityToken API in CMSMain->publishall() (fixes #6159)
[rev:113281] Removed unused !SecurityAdmin->!MemberForm() and savemember() (see !MemberTableField)
[rev:113280] Removed unused Security->addmember() (see !MemberTableField and !SecurityAdmin->addtogroup())
[rev:113279] Removed unused !SecurityAdmin->removememberfromgroup() (see !MemberTableField)
[rev:113278] Removed unused !MemberList templates (see !MemberTableField)
[rev:113274] Using !SecurityToken in !ViewableData->getSecurityID()
[rev:113248] Javascript translations in CMSMain_right.js (fixes #6142)
[rev:113241] Documentation
[rev:112982] updated typo in comment for Cache.
[rev:112962] Fix to !SapphireInfo for git-svn checkouts.
[rev:112961] Add documentation to File::$allowed_extensions explaining that there are config files to edit in assets/
[rev:112321] Removed "In line of " text in CLI test reporter which did not work. Details are in the backtrace below anyway, so it's not required
[rev:112278] Reverted regression in r112272
[rev:112254] change the requirement's link to use current protocol (we don't want messages from browsers saying the page has unsecured content, when accessing the CMS over SSL)
[rev:111950] Comment about HTMLValue::setContent() stripping out of carriage returns
[rev:111903] #6083 !FileTest doesn't remove test folders and files created during test
[rev:111899] Use Filesystem::removeFolder() in !FilesystemPublisherTest::tearDown() instead of specific code to handle this
[rev:111898] Code syntax formatting of Filesystem::removeFolder()
[rev:111888] Moved GD::set_default_quality() function to the top of the file to align with conventions
[rev:111883] #6090 !FilesystemPublisherTest now stores temporary files in assets, which is writable, instead of the webroot which almost never has write permissions
[rev:111875] Enable non-default language for tinyMCE, setting language in _config.php didn't work. Thanks for @christian
[rev:111852] Revert r111850 to !MySQLDatabase::getVersion as version comparisons need to happen, and this will strip out non-numeric characters e.g. "ubuntu1" or "lenny4" which are prefixed on some Linux distros
[rev:111851] dev/build now shows database name and version next to "Building database ..." text
[rev:111844] Fixed regression from r111843 (i18nText, !MemberDatetimeFieldTest, !MemberTest)
[rev:111843] Fixed form validation message in !DateField to include actual date format, rather than a hardcoded value
[rev:111821] Change matchesRoughly threshold slightly in !DbDatetimeTest to allow for slower database server connections
[rev:111789] Added !FulltextSearchable::get_searchable_classes() in order to introspect currently searchable classes, added !FulltextSearchableTest, added documentation
[rev:111788] Fixed documentation in !CheckboxSetField (fixes #6068, thanks paradigmincarnate)
[rev:111787] Fixed documentation in Datetime (fixes #6062, thanks nicolaas)
[rev:111786] Fixed SS_Datetime references in !BrokenLinksReport and !CommentAdmin (fixes #6063, thanks nicolaas)
[rev:111772] Code formatting tidy of DB::connect() function
[rev:111748] CoreTest::testGetTempPathInProject() will try to create a temp dirs when running. !CoreTest::tearDown() will now remove these temp dirs when the test finishes
[rev:111676] #5943 Debug::text() boolean values are amended with (bool) so they don't get confused with "true" or "false" which could be strings (thanks Pigeon!)
[rev:111669] Unit test breaks if another module or project extends Folder
[rev:111597] Updated language master file
[rev:111497] Fixed indentation in !PageCommentInterface.js
[rev:111496] Fixed SQL quoting bug in !FolderTest (caused by r111493)
[rev:111454] removed debug
[rev:111450] removed debug
[rev:111262] Add translation correction for Czech and add Slovakian translation in cms and sapphire (js). Thanks to @Pike
[rev:111224] Ensuring !SiteTreeAccess.js is properly minified in live mode
[rev:111133] Code formatting in !FullTestSuite
[rev:111123] Spelling corrections to Director comments
[rev:111116] PHPUnit annotations for !PhpSyntaxTest
[rev:111053] Removing !MemberImportFormTest, breaks PHPUnit test run, and doesnt have any assertions
[rev:111052] Documentation for constants in Core.php
[rev:111051] Don't use chdir(), it confuses the hell out of phpunit (e.g. directory_exists() and realpath() no longer work as expected)
[rev:111047] Fixed SSViewerTest to initialize controller properly
[rev:111046] Remove all session data in !TestSession that might've been set by the test harness (necessary for test runs through the phpunit binary)
[rev:111042] added phpdoc to the new PHPUnitWrapper classes.

Other

[rev:111880] #4029 On the fly form validation works in Opera as well
[rev:111879] Added doc for static help_link
[rev:111452] Fixes #2782
[rev:111040] API-CHANGE: remove include which is not required.
[rev:111038] ENHACENEMENT: Change behaviour of the !MenufestBuilder to use spl_autoload_register instead of traditional __autoload.

sscreatechangelog --version 2.4.3 --branch branches/2.4 --stopbranch tags/2.4.2

21 KiB Raw Blame History