From 0e443bafa0c8c87c91f5344142418f32a3e5f853 Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Mon, 8 Jul 2013 15:27:13 +1200 Subject: [PATCH 01/16] Deprecate Aggregate and DataObject::getComponentsQuery --- model/Aggregate.php | 7 +++++++ model/DataObject.php | 34 +++------------------------------- 2 files changed, 10 insertions(+), 31 deletions(-) diff --git a/model/Aggregate.php b/model/Aggregate.php index f0dce851b..372843299 100644 --- a/model/Aggregate.php +++ b/model/Aggregate.php @@ -29,6 +29,8 @@ * NOTE: The cache logic uses tags, and so a backend that supports tags is required. Currently only the File * backend (and the two-level backend with the File backend as the slow store) meets this requirement * + * @deprecated 3.1 Use DataList to aggregate data + * * @author hfried * @package framework * @subpackage core @@ -60,10 +62,15 @@ class Aggregate extends ViewableData { /** * Constructor * + * @deprecated 3.1 Use DataList to aggregate data + * * @param string $type The DataObject type we are building an aggregate for * @param string $filter (optional) An SQL filter to apply to the selected rows before calculating the aggregate */ public function __construct($type, $filter = '') { + Deprecation::notice('3.1', 'Call aggregate methods on a DataList directly instead. In templates' + . ' an example of the new syntax is <% cached List(Member).max(LastEdited) %> instead' + . ' (check partial-caching.md documentation for more details.)'); $this->type = $type; $this->filter = $filter; parent::__construct(); diff --git a/model/DataObject.php b/model/DataObject.php index beb53fb14..3d7a730fe 100644 --- a/model/DataObject.php +++ b/model/DataObject.php @@ -1468,39 +1468,11 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity } /** - * Get the query object for a $has_many Component. - * - * @param string $componentName - * @param string $filter - * @param string|array $sort - * @param string $join Deprecated, use leftJoin($table, $joinClause) instead - * @param string|array $limit - * @return SQLQuery + * @deprecated 3.1 Use getComponents to get a filtered DataList for an object's relation */ public function getComponentsQuery($componentName, $filter = "", $sort = "", $join = "", $limit = "") { - if(!$componentClass = $this->has_many($componentName)) { - user_error("DataObject::getComponentsQuery(): Unknown 1-to-many component '$componentName'" - . " on class '$this->class'", E_USER_ERROR); - } - - if($join) { - throw new \InvalidArgumentException( - 'The $join argument has been removed. Use leftJoin($table, $joinClause) instead.' - ); - } - - $joinField = $this->getRemoteJoinField($componentName, 'has_many'); - - $id = $this->getField("ID"); - - // get filter - $combinedFilter = "\"$joinField\" = '$id'"; - if(!empty($filter)) $combinedFilter .= " AND ({$filter})"; - - return DataList::create($componentClass) - ->where($combinedFilter) - ->canSortBy($sort) - ->limit($limit); + Deprecation::notice('3.1', "Use getComponents to get a filtered DataList for an object's relation"); + return $this->getComponents($componentName, $filter, $sort, $join, $limit); } /** From 7b7982969b8476ed1455a7fbe23c7e45f9946449 Mon Sep 17 00:00:00 2001 From: Hamish Friedlander Date: Wed, 10 Jul 2013 16:24:50 +1200 Subject: [PATCH 02/16] Add some docs about admin-side HTML sanitisation --- docs/en/topics/security.md | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/docs/en/topics/security.md b/docs/en/topics/security.md index acc9ba89a..59e8deaf6 100644 --- a/docs/en/topics/security.md +++ b/docs/en/topics/security.md @@ -127,6 +127,38 @@ or [sanitize](http://htmlpurifier.org/) it correctly. See [http://shiflett.org/articles/foiling-cross-site-attacks](http://shiflett.org/articles/foiling-cross-site-attacks) for in-depth information about "Cross-Site-Scripting". +### What if I can't trust my editors? + +The default configuration of SilverStripe assumes some level of trust is given to your editors who have access +to the CMS. Though the HTML WYSIWYG editor is configured to provide some control over the HTML an editor provides, +this is not enforced server side, and so can be bypassed by a malicious editor. A editor that does so can use an +XSS attack against an admin to perform any administrative action. + +If you can't trust your editors, SilverStripe must be configured to filter the content so that any javascript is +stripped out + +To enable filtering, set the HtmlEditorField::$sanitise_server_side [configuration](/topics/configuration) property to +true, e.g. + + HtmlEditorField::config()->sanitise_server_side = true + +The built in sanitiser enforces the TinyMCE whitelist rules on the server side, and is sufficient to eliminate the +most common XSS vectors. + +However some subtle XSS attacks that exploit HTML parsing bugs need heavier filtering. For greater protection +you can install the [htmlpurifier](https://github.com/silverstripe-labs/silverstripe-htmlpurifier) module which +will replace the built in sanitiser with one that uses the [HTML Purifier](http://htmlpurifier.org/) library. + +In both cases, you must ensure that you have not configured TinyMCE to explicitly allow script elements or other +javascript-specific attributes. + +##### But I also need my editors to provide javascript + +It is not currently possible to allow editors to provide javascript content and yet still protect other users +from any malicious code within that javascript. + +We recommend configuring [shortcodes](/reference/shortcodes) that can be used by editors in place of using javascript directly. + ### Escaping model properties `[api:SSViewer]` (the SilverStripe template engine) automatically takes care of escaping HTML tags from specific From 378d829e8fce8eab9b0a9af9ffe74585e00e3dca Mon Sep 17 00:00:00 2001 From: Daniel Hensby Date: Wed, 10 Jul 2013 12:47:13 +0100 Subject: [PATCH 03/16] Adding test to prove issue with HTTP Header parsing in RestfulService I have a header like: X-BB-Auth: xxxx and it is being given back to me as X-Bb-Auth - i want to prove the issue and the fix --- tests/api/RestfulServiceTest.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/api/RestfulServiceTest.php b/tests/api/RestfulServiceTest.php index a9248db3a..772ccd9ef 100644 --- a/tests/api/RestfulServiceTest.php +++ b/tests/api/RestfulServiceTest.php @@ -172,12 +172,14 @@ class RestfulServiceTest extends SapphireTest { public function testHttpHeaderParseing() { $headers = "content-type: text/html; charset=UTF-8\r\n". "Server: Funky/1.0\r\n". + "X-BB-ExampleMANycaPS: test\r\n". "Set-Cookie: foo=bar\r\n". "Set-Cookie: baz=quux\r\n". "Set-Cookie: bar=foo\r\n"; $expected = array( 'Content-Type' => 'text/html; charset=UTF-8', 'Server' => 'Funky/1.0', + 'X-BB-ExampleMANycaPS' => 'test', 'Set-Cookie' => array( 'foo=bar', 'baz=quux', From ddd6a15b4a6906b8ea23d04cb0f6415aac9650af Mon Sep 17 00:00:00 2001 From: Daniel Hensby Date: Wed, 10 Jul 2013 13:00:40 +0100 Subject: [PATCH 04/16] FIX RestfulService header parsing now accepts non-title case headers --- api/RestfulService.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/RestfulService.php b/api/RestfulService.php index 05913c3f5..5c95d451d 100644 --- a/api/RestfulService.php +++ b/api/RestfulService.php @@ -352,7 +352,7 @@ class RestfulService extends ViewableData { $match[1] = preg_replace_callback( '/(?<=^|[\x09\x20\x2D])./', create_function('$matches', 'return strtoupper($matches[0]);'), - strtolower(trim($match[1])) + trim($match[1]) ); if( isset($headers[$match[1]]) ) { if (!is_array($headers[$match[1]])) { From b58e2dbe3a1b94c6f69c6b708761f1dce71c0429 Mon Sep 17 00:00:00 2001 From: Ingo Schommer Date: Thu, 11 Jul 2013 01:17:02 +0200 Subject: [PATCH 05/16] Member.lock_out_delay_mins configurable, password security docs --- docs/en/topics/security.md | 34 ++++++++++++--- lang/en.yml | 1 + security/Member.php | 28 +++++++++--- tests/security/SecurityTest.php | 77 ++++++++++++++++++++------------- 4 files changed, 97 insertions(+), 43 deletions(-) diff --git a/docs/en/topics/security.md b/docs/en/topics/security.md index 59e8deaf6..3e64d5b6c 100644 --- a/docs/en/topics/security.md +++ b/docs/en/topics/security.md @@ -407,13 +407,37 @@ configuration and test fixtures). You should therefore block access to all yaml files (extension .yml) by default, and white list only yaml files you need to serve directly. -See [Apache](/installation/webserver) and [Nginx](/installation/nginx) installation documentation for details -specific to your web server +See [Apache](/installation/webserver) and [Nginx](/installation/nginx) installation documentation for details specific to your web server + +## Passwords + +SilverStripe stores passwords with a strong hashing algorithm (blowfish) by default +(see [api:PasswordEncryptor]). It adds randomness to these hashes via +salt values generated with the strongest entropy generators available on the platform +(see [api:RandomGenerator]). This prevents brute force attacks with +[Rainbow tables](http://en.wikipedia.org/wiki/Rainbow_table). + +Strong passwords are a crucial part of any system security. +So in addition to storing the password in a secure fashion, +you can also enforce specific password policies by configuring +a [api:PasswordValidator]: + + :::php + $validator = new PasswordValidator(); + $validator->minLength(7); + $validator->checkHistoricalPasswords(6); + $validator->characterStrength('lowercase','uppercase','digits','punctuation'); + Member::set_password_validator($validator); + +In addition, you can tighten password security with the following configuration settings: + + * `Member.password_expiry_days`: Set the number of days that a password should be valid for. + * `Member.lock_out_after_incorrect_logins`: Number of incorrect logins after which + the user is blocked from further attempts for the timespan defined in `$lock_out_delay_mins` + * `Member.lock_out_delay_mins`: Minutes of enforced lockout after incorrect password attempts. + Only applies if `lock_out_after_incorrect_logins` is greater than 0. ## Related * [http://silverstripe.org/security-releases/](http://silverstripe.org/security-releases/) - -## Links - * [Best-practices for securing MySQL (securityfocus.com)](http://www.securityfocus.com/infocus/1726) diff --git a/lang/en.yml b/lang/en.yml index 64df41040..154dc2ad4 100644 --- a/lang/en.yml +++ b/lang/en.yml @@ -367,6 +367,7 @@ en: EMPTYNEWPASSWORD: 'The new password can''t be empty, please try again' ENTEREMAIL: 'Please enter an email address to get a password reset link.' ERRORLOCKEDOUT: 'Your account has been temporarily disabled because of too many failed attempts at logging in. Please try again in 20 minutes.' + ERRORLOCKEDOUT2: 'Your account has been temporarily disabled because of too many failed attempts at logging in. Please try again in {count} minutes.' ERRORNEWPASSWORD: 'You have entered your new password differently, try again' ERRORPASSWORDNOTMATCH: 'Your current password does not match, please try again' ERRORWRONGCRED: 'That doesn''t seem to be the right e-mail address or password. Please try again.' diff --git a/security/Member.php b/security/Member.php index 3afbd5361..90afb5101 100644 --- a/security/Member.php +++ b/security/Member.php @@ -113,9 +113,18 @@ class Member extends DataObject implements TemplateGlobalProvider { /** * @config - * @var Int + * @var Int Number of incorrect logins after which + * the user is blocked from further attempts for the timespan + * defined in {@link $lock_out_delay_mins}. */ private static $lock_out_after_incorrect_logins = null; + + /** + * @config + * @var integer Minutes of enforced lockout after incorrect password attempts. + * Only applies if {@link $lock_out_after_incorrect_logins} greater than 0. + */ + private static $lock_out_delay_mins = 15; /** * @config @@ -238,11 +247,15 @@ class Member extends DataObject implements TemplateGlobalProvider { $result = new ValidationResult(); if($this->isLockedOut()) { - $result->error(_t ( - 'Member.ERRORLOCKEDOUT', - 'Your account has been temporarily disabled because of too many failed attempts at ' . - 'logging in. Please try again in 20 minutes.' - )); + $result->error( + _t( + 'Member.ERRORLOCKEDOUT2', + 'Your account has been temporarily disabled because of too many failed attempts at ' . + 'logging in. Please try again in {count} minutes.', + null, + array('count' => $this->config()->lock_out_delay_mins) + ) + ); } $this->extend('canLogIn', $result); @@ -1407,7 +1420,8 @@ class Member extends DataObject implements TemplateGlobalProvider { $this->write(); if($this->FailedLoginCount >= self::config()->lock_out_after_incorrect_logins) { - $this->LockedOutUntil = date('Y-m-d H:i:s', time() + 15*60); + $lockoutMins = self::config()->lock_out_delay_mins; + $this->LockedOutUntil = date('Y-m-d H:i:s', time() + $lockoutMins*60); $this->write(); } } diff --git a/tests/security/SecurityTest.php b/tests/security/SecurityTest.php index 294b082b4..0f76c7ac6 100644 --- a/tests/security/SecurityTest.php +++ b/tests/security/SecurityTest.php @@ -250,61 +250,76 @@ class SecurityTest extends FunctionalTest { i18n::set_locale('en_US'); Member::config()->lock_out_after_incorrect_logins = 5; + Member::config()->lock_out_delay_mins = 15; - /* LOG IN WITH A BAD PASSWORD 7 TIMES */ - - for($i=1;$i<=7;$i++) { + // Login with a wrong password for more than the defined threshold + for($i = 1; $i <= Member::config()->lock_out_after_incorrect_logins+1; $i++) { $this->doTestLoginForm('sam@silverstripe.com' , 'incorrectpassword'); $member = DataObject::get_by_id("Member", $this->idFromFixture('Member', 'test')); - /* THE FIRST 4 TIMES, THE MEMBER SHOULDN'T BE LOCKED OUT */ - if($i < 5) { - $this->assertNull($member->LockedOutUntil); + if($i < Member::config()->lock_out_after_incorrect_logins) { + $this->assertNull( + $member->LockedOutUntil, + 'User does not have a lockout time set if under threshold for failed attempts' + ); $this->assertContains($this->loginErrorMessage(), _t('Member.ERRORWRONGCRED')); + } else { + // Fuzzy matching for time to avoid side effects from slow running tests + $this->assertGreaterThan( + time() + 14*60, + strtotime($member->LockedOutUntil), + 'User has a lockout time set after too many failed attempts' + ); } - - /* AFTER THAT THE USER IS LOCKED OUT FOR 15 MINUTES */ - //(we check for at least 14 minutes because we don't want a slow running test to report a failure.) - else { - $this->assertGreaterThan(time() + 14*60, strtotime($member->LockedOutUntil)); - } - - if($i > 5) { - $this->assertContains(_t('Member.ERRORLOCKEDOUT'), $this->loginErrorMessage()); - // $this->assertTrue(false !== stripos($this->loginErrorMessage(), _t('Member.ERRORLOCKEDOUT'))); + $msg = _t( + 'Member.ERRORLOCKEDOUT2', + 'Your account has been temporarily disabled because of too many failed attempts at ' . + 'logging in. Please try again in {count} minutes.', + null, + array('count' => Member::config()->lock_out_delay_mins) + ); + if($i > Member::config()->lock_out_after_incorrect_logins) { + $this->assertContains($msg, $this->loginErrorMessage()); } } - /* THE USER CAN'T LOG IN NOW, EVEN IF THEY GET THE RIGHT PASSWORD */ - $this->doTestLoginForm('sam@silverstripe.com' , '1nitialPassword'); - $this->assertNull($this->session()->inst_get('loggedInAs')); + $this->assertNull( + $this->session()->inst_get('loggedInAs'), + 'The user can\'t log in after being locked out, even with the right password' + ); - /* BUT, IF TIME PASSES, THEY CAN LOG IN */ - // (We fake this by re-setting LockedOutUntil) $member = DataObject::get_by_id("Member", $this->idFromFixture('Member', 'test')); $member->LockedOutUntil = date('Y-m-d H:i:s', time() - 30); $member->write(); - $this->doTestLoginForm('sam@silverstripe.com' , '1nitialPassword'); - $this->assertEquals($this->session()->inst_get('loggedInAs'), $member->ID); + $this->assertEquals( + $this->session()->inst_get('loggedInAs'), + $member->ID, + 'After lockout expires, the user can login again' + ); // Log the user out $this->session()->inst_set('loggedInAs', null); - /* NOW THAT THE LOCK-OUT HAS EXPIRED, CHECK THAT WE ARE ALLOWED 4 FAILED ATTEMPTS BEFORE LOGGING IN */ - - $this->doTestLoginForm('sam@silverstripe.com' , 'incorrectpassword'); - $this->doTestLoginForm('sam@silverstripe.com' , 'incorrectpassword'); - $this->doTestLoginForm('sam@silverstripe.com' , 'incorrectpassword'); - $this->doTestLoginForm('sam@silverstripe.com' , 'incorrectpassword'); + // Login again with wrong password, but less attempts than threshold + for($i = 1; $i < Member::config()->lock_out_after_incorrect_logins; $i++) { + $this->doTestLoginForm('sam@silverstripe.com' , 'incorrectpassword'); + } $this->assertNull($this->session()->inst_get('loggedInAs')); - $this->assertTrue(false !== stripos($this->loginErrorMessage(), _t('Member.ERRORWRONGCRED'))); + $this->assertTrue( + false !== stripos($this->loginErrorMessage(), _t('Member.ERRORWRONGCRED')), + 'The user can retry with a wrong password after the lockout expires' + ); $this->doTestLoginForm('sam@silverstripe.com' , '1nitialPassword'); - $this->assertEquals($this->session()->inst_get('loggedInAs'), $member->ID); + $this->assertEquals( + $this->session()->inst_get('loggedInAs'), + $member->ID, + 'The user can login successfully after lockout expires, if staying below the threshold' + ); i18n::set_locale($local); } From c2c8498c642dc59b177c95317a016deb2d33fcf0 Mon Sep 17 00:00:00 2001 From: Ingo Schommer Date: Thu, 11 Jul 2013 15:13:27 +0200 Subject: [PATCH 06/16] BehatFixtureFactory 5.3.8 compat (wrong usage of is_a()) --- dev/BehatFixtureFactory.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dev/BehatFixtureFactory.php b/dev/BehatFixtureFactory.php index b4ce190de..a4f7a829a 100644 --- a/dev/BehatFixtureFactory.php +++ b/dev/BehatFixtureFactory.php @@ -9,7 +9,7 @@ class BehatFixtureFactory extends \FixtureFactory { // Copy identifier to some visible property unless its already defined. // Exclude files, since they generate their own named based on the file path. - if(!is_a($name, 'File', true)) { + if(!$name != 'File' && !is_subclass_of($name, 'File')) { foreach(array('Name', 'Title') as $fieldName) { if(singleton($name)->hasField($fieldName) && !isset($data[$fieldName])) { $data[$fieldName] = $identifier; From 71f8c1306f23af39062a173bbaca28b14a8a83c3 Mon Sep 17 00:00:00 2001 From: Jeremy Thomerson Date: Thu, 11 Jul 2013 13:40:34 +0000 Subject: [PATCH 07/16] DOCFIX: small typo causing linking error --- docs/en/reference/grid-field.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/en/reference/grid-field.md b/docs/en/reference/grid-field.md index 06326568b..d21f443a4 100644 --- a/docs/en/reference/grid-field.md +++ b/docs/en/reference/grid-field.md @@ -154,7 +154,7 @@ The fields displayed in the edit form are from `DataObject::getCMSFields()` The `GridFieldDetailForm` component drives the record editing form which is usually configured through the configs `GridFieldConfig_RecordEditor` and `GridFieldConfig_RelationEditor` described above. It takes its fields from `DataObject->getCMSFields()`, -but can be customized to accept different fields via its `[api:GridFieldDetailForm->setFields()](api:setFields())` method. +but can be customized to accept different fields via its `[api:GridFieldDetailForm->setFields()]` method. The component also has the ability to load and save data stored on join tables when two records are related via a "many_many" relationship, as defined through From 7fbc7527648998c0b4c1a9207c79861ef5ff876d Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Fri, 12 Jul 2013 15:07:43 +1200 Subject: [PATCH 08/16] Typo --- model/fieldtypes/ForeignKey.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/model/fieldtypes/ForeignKey.php b/model/fieldtypes/ForeignKey.php index 3fe7b07f3..1c51529f3 100644 --- a/model/fieldtypes/ForeignKey.php +++ b/model/fieldtypes/ForeignKey.php @@ -7,7 +7,7 @@ * @uses DropdownField * * @param string $name - * @param DataOject $object The object that the foreign key is stored on (should have a relation with $name) + * @param DataObject $object The object that the foreign key is stored on (should have a relation with $name) * * @package framework * @subpackage model From 920edf88e7b29fe25345d060cea8ef19f1291194 Mon Sep 17 00:00:00 2001 From: Ingo Schommer Date: Fri, 12 Jul 2013 13:16:25 +0200 Subject: [PATCH 09/16] Test allowedExtensions in UploadField, return correct HTTP status --- forms/UploadField.php | 2 ++ tests/forms/uploadfield/UploadFieldTest.php | 33 ++++++++++++++++++++- 2 files changed, 34 insertions(+), 1 deletion(-) diff --git a/forms/UploadField.php b/forms/UploadField.php index 2766b028f..cf02fe676 100644 --- a/forms/UploadField.php +++ b/forms/UploadField.php @@ -525,6 +525,7 @@ class UploadField extends FileField { } // Get the uploaded file into a new file object. + // The loadIntoFile() method also validates constraints like allowed extensions try { $this->upload->loadIntoFile($tmpfile, $fileObject, $this->folderName); } catch (Exception $e) { @@ -559,6 +560,7 @@ class UploadField extends FileField { } $response = new SS_HTTPResponse(Convert::raw2json(array($return))); $response->addHeader('Content-Type', 'text/plain'); + if($return['error']) $response->setStatusCode(403); return $response; } diff --git a/tests/forms/uploadfield/UploadFieldTest.php b/tests/forms/uploadfield/UploadFieldTest.php index 9543414ab..6da1d3844 100644 --- a/tests/forms/uploadfield/UploadFieldTest.php +++ b/tests/forms/uploadfield/UploadFieldTest.php @@ -123,6 +123,33 @@ class UploadFieldTest extends FunctionalTest { $this->assertEquals($record->ManyManyFiles()->Last()->Name, $tmpFileName); } + /** + * Partially covered by {@link UploadTest->testUploadAcceptsAllowedExtension()}, + * but this test additionally verifies that those constraints are actually enforced + * in this controller method. + */ + public function testAllowedExtensions() { + $this->loginWithPermission('ADMIN'); + + $invalidFile = 'invalid.php'; + $_FILES = array('AllowedExtensionsField' => $this->getUploadFile($invalidFile)); + $response = $this->post( + 'UploadFieldTest_Controller/Form/field/AllowedExtensionsField/upload', + array('AllowedExtensionsField' => $this->getUploadFile($invalidFile)) + ); + $this->assertTrue($response->isError()); + $this->assertContains('Extension is not allowed', $response->getBody()); + + $validFile = 'valid.jpg'; + $_FILES = array('AllowedExtensionsField' => $this->getUploadFile($validFile)); + $response = $this->post( + 'UploadFieldTest_Controller/Form/field/AllowedExtensionsField/upload', + array('AllowedExtensionsField' => $this->getUploadFile($validFile)) + ); + $this->assertFalse($response->isError()); + $this->assertNotContains('Extension is not allowed', $response->getBody()); + } + public function testAllowedMaxFileNumberWithHasOne() { $this->loginWithPermission('ADMIN'); @@ -831,6 +858,9 @@ class UploadFieldTest_Controller extends Controller implements TestOnly { $fieldCanAttachExisting->setConfig('canAttachExisting', false); $fieldCanAttachExisting->setRecord($record); + $fieldAllowedExtensions = new UploadField('AllowedExtensionsField'); + $fieldAllowedExtensions->getValidator()->setAllowedExtensions(array('jpg')); + $form = new Form( $this, 'Form', @@ -847,7 +877,8 @@ class UploadFieldTest_Controller extends Controller implements TestOnly { $fieldDisabled, $fieldSubfolder, $fieldCanUploadFalse, - $fieldCanAttachExisting + $fieldCanAttachExisting, + $fieldAllowedExtensions ), new FieldList( new FormAction('submit') From 2427d57fa54659f7ee17b79c4a31deec036ce9a3 Mon Sep 17 00:00:00 2001 From: ARNHOE Date: Sat, 13 Jul 2013 13:00:46 +0200 Subject: [PATCH 10/16] Updated loop/if/with to be more consistent --- admin/templates/CMSBreadcrumbs.ss | 8 +++--- .../templates/CMSProfileController_Content.ss | 12 ++++---- admin/templates/CMSTabSet.ss | 6 ++-- admin/templates/Includes/BackLink_Button.ss | 4 +-- admin/templates/Includes/CMSSectionIcon.ss | 6 ++-- admin/templates/Includes/Editor_toolbar.ss | 12 ++++---- .../Includes/LeftAndMain_EditForm.ss | 28 +++++++++---------- admin/templates/Includes/LeftAndMain_Menu.ss | 14 +++++----- .../LeftAndMain_SilverStripeNavigator.ss | 16 +++++------ .../templates/Includes/ModelAdmin_Content.ss | 4 +-- .../Includes/ModelAdmin_ImportSpec.ss | 4 +-- admin/templates/Includes/ModelAdmin_Tools.ss | 2 +- admin/templates/ModelSidebar.ss | 4 +-- templates/Image_iframe.ss | 8 +++--- templates/Includes/Form.ss | 14 +++++----- .../GridFieldAddExistingAutocompleter.ss | 2 +- .../Includes/GridFieldFilterHeader_Row.ss | 2 +- templates/Includes/GridFieldFooter.ss | 4 +-- templates/Includes/GridFieldItemEditView.ss | 4 +-- .../Includes/GridFieldSortableHeader_Row.ss | 2 +- templates/Includes/GridFieldToolbarHeader.ss | 4 +-- templates/Includes/GridField_FormAction.ss | 4 +-- templates/Includes/GridField_Item.ss | 4 +-- templates/Includes/GridField_print.ss | 8 +++--- .../Includes/HtmlEditorField_viewfile.ss | 4 +-- templates/Includes/UploadField_FileButtons.ss | 6 ++-- templates/RSSFeed.ss | 8 +++--- templates/RightLabelledFieldHolder.ss | 4 +-- templates/SearchForm.ss | 10 +++---- templates/UploadField.ss | 12 ++++---- templates/forms/CheckboxField_holder.ss | 4 +-- templates/forms/CheckboxField_holder_small.ss | 4 +-- templates/forms/CheckboxSetField.ss | 6 ++-- templates/forms/CompositeField.ss | 6 ++-- templates/forms/CompositeField_holder.ss | 8 +++--- templates/forms/DropdownField.ss | 4 +-- templates/forms/FieldGroup.ss | 6 ++-- .../forms/FieldGroup_DefaultFieldHolder.ss | 6 ++-- templates/forms/FieldGroup_holder.ss | 12 ++++---- templates/forms/FormAction.ss | 6 ++-- templates/forms/FormField.ss | 4 +-- templates/forms/FormField_holder.ss | 12 ++++---- templates/forms/FormField_holder_small.ss | 8 +++--- templates/forms/OptionsetField.ss | 4 +-- templates/forms/SelectionGroup.ss | 22 +++++++++------ templates/forms/TabSet.ss | 8 +++--- templates/forms/TreeDropdownField.ss | 6 ++-- 47 files changed, 176 insertions(+), 170 deletions(-) diff --git a/admin/templates/CMSBreadcrumbs.ss b/admin/templates/CMSBreadcrumbs.ss index 9ef793482..e0d9f13e5 100644 --- a/admin/templates/CMSBreadcrumbs.ss +++ b/admin/templates/CMSBreadcrumbs.ss @@ -1,15 +1,15 @@

- <% if ToplevelController %> + <% if $ToplevelController %> - <% else_if Controller %> + <% else_if $Controller %> <% else %> <% end_if %> - <% loop Breadcrumbs %> - <% if Last %> + <% loop $Breadcrumbs %> + <% if $Last %> $Title.XML <% else %> $Title.XML diff --git a/admin/templates/CMSProfileController_Content.ss b/admin/templates/CMSProfileController_Content.ss index 1def543a3..d7675888e 100644 --- a/admin/templates/CMSProfileController_Content.ss +++ b/admin/templates/CMSProfileController_Content.ss @@ -1,18 +1,18 @@
- <% with EditForm %> + <% with $EditForm %>
- <% with Controller %> + <% with $Controller %> <% include CMSBreadcrumbs %> <% end_with %>
- <% if Fields.hasTabset %> - <% with Fields.fieldByName('Root') %> + <% if $Fields.hasTabset %> + <% with $Fields.fieldByName('Root') %>
    - <% loop Tabs %> - class="$extraClass"<% end_if %>>$Title + <% loop $Tabs %> + class="$extraClass"<% end_if %>>$Title <% end_loop %>
diff --git a/admin/templates/CMSTabSet.ss b/admin/templates/CMSTabSet.ss index 7a64cfe6f..4e232624c 100644 --- a/admin/templates/CMSTabSet.ss +++ b/admin/templates/CMSTabSet.ss @@ -5,12 +5,12 @@ <%-- e.g. through LeftAndMain_EditForm.ss. --%>
- <% loop Tabs %> - <% if Tabs %> + <% loop $Tabs %> + <% if $Tabs %> $FieldHolder <% else %>
- <% loop Fields %> + <% loop $Fields %> $FieldHolder <% end_loop %>
diff --git a/admin/templates/Includes/BackLink_Button.ss b/admin/templates/Includes/BackLink_Button.ss index cdca8ff0e..d6217a98f 100644 --- a/admin/templates/Includes/BackLink_Button.ss +++ b/admin/templates/Includes/BackLink_Button.ss @@ -1,7 +1,7 @@ -<% if Backlink %> +<% if $Backlink %>
<% _t('BackLink_Button.ss.Back', 'Back') %>
-<% end_if %> \ No newline at end of file +<% end_if %> diff --git a/admin/templates/Includes/CMSSectionIcon.ss b/admin/templates/Includes/CMSSectionIcon.ss index 7402c7f9d..463981c54 100644 --- a/admin/templates/Includes/CMSSectionIcon.ss +++ b/admin/templates/Includes/CMSSectionIcon.ss @@ -1,7 +1,7 @@ -<% if ToplevelController %> +<% if $ToplevelController %> -<% else_if Controller %> +<% else_if $Controller %> <% else %> -<% end_if %> \ No newline at end of file +<% end_if %> diff --git a/admin/templates/Includes/Editor_toolbar.ss b/admin/templates/Includes/Editor_toolbar.ss index 773877982..6a5be7c28 100644 --- a/admin/templates/Includes/Editor_toolbar.ss +++ b/admin/templates/Includes/Editor_toolbar.ss @@ -1,20 +1,20 @@ -<% with EditorToolbar %> +<% with $EditorToolbar %>
- <% loop Fields %> + <% loop $Fields %> <% end_loop %> diff --git a/templates/Includes/GridFieldFooter.ss b/templates/Includes/GridFieldFooter.ss index f4b47d73d..c464d936c 100644 --- a/templates/Includes/GridFieldFooter.ss +++ b/templates/Includes/GridFieldFooter.ss @@ -7,8 +7,8 @@ $NumRecords - <% if Message %> + <% if $Message %>
$Message
<% end_if %> - \ No newline at end of file + diff --git a/templates/Includes/GridFieldItemEditView.ss b/templates/Includes/GridFieldItemEditView.ss index 8a6a025b8..20b780413 100644 --- a/templates/Includes/GridFieldItemEditView.ss +++ b/templates/Includes/GridFieldItemEditView.ss @@ -1,5 +1,5 @@ -<% if Backlink %> +<% if $Backlink %> <% _t('Go back', 'Go back' ) %> <% end_if %> -$ItemEditForm \ No newline at end of file +$ItemEditForm diff --git a/templates/Includes/GridFieldSortableHeader_Row.ss b/templates/Includes/GridFieldSortableHeader_Row.ss index 0197407b7..2b12b728a 100644 --- a/templates/Includes/GridFieldSortableHeader_Row.ss +++ b/templates/Includes/GridFieldSortableHeader_Row.ss @@ -1,5 +1,5 @@ - <% loop Fields %> + <% loop $Fields %> <% end_loop %> diff --git a/templates/Includes/GridFieldToolbarHeader.ss b/templates/Includes/GridFieldToolbarHeader.ss index 27aabb79a..6c94910a3 100644 --- a/templates/Includes/GridFieldToolbarHeader.ss +++ b/templates/Includes/GridFieldToolbarHeader.ss @@ -1,7 +1,7 @@ - \ No newline at end of file + diff --git a/templates/Includes/GridField_FormAction.ss b/templates/Includes/GridField_FormAction.ss index 90e1cf96c..039f1fd34 100644 --- a/templates/Includes/GridField_FormAction.ss +++ b/templates/Includes/GridField_FormAction.ss @@ -1,3 +1,3 @@ \ No newline at end of file + <% if $ButtonContent %>$ButtonContent<% else %>$Title<% end_if %> + diff --git a/templates/Includes/GridField_Item.ss b/templates/Includes/GridField_Item.ss index 2128f53af..ed5a5fe15 100644 --- a/templates/Includes/GridField_Item.ss +++ b/templates/Includes/GridField_Item.ss @@ -1,11 +1,11 @@ <% if $GridField.ExtraColumnsCount %> - <% loop Fields %> + <% loop $Fields %> <% end_loop %> <% else %> - <% loop Fields %> + <% loop $Fields %> <% end_loop %> <% end_if %> diff --git a/templates/Includes/GridField_print.ss b/templates/Includes/GridField_print.ss index 8595007fd..be23e8bba 100644 --- a/templates/Includes/GridField_print.ss +++ b/templates/Includes/GridField_print.ss @@ -7,12 +7,12 @@ <% if $Title %>

$Title

<% end_if %>
-<% loop Buttons %> - <% if Type = button %> +<% loop $Buttons %> + <% if $Type = button %> $Title - <% else_if Type = dropdown %> + <% else_if $Type = dropdown %> - <% else_if Type = separator %> + <% else_if $Type = separator %> | - <% else_if Type = break %> + <% else_if $Type = break %>
<% end_if %> <% end_loop %> diff --git a/admin/templates/Includes/LeftAndMain_EditForm.ss b/admin/templates/Includes/LeftAndMain_EditForm.ss index 1274f741e..3de5007ec 100644 --- a/admin/templates/Includes/LeftAndMain_EditForm.ss +++ b/admin/templates/Includes/LeftAndMain_EditForm.ss @@ -1,19 +1,19 @@ -<% if IncludeFormTag %> +<% if $IncludeFormTag %>
<% end_if %>
<% include BackLink_Button %> - <% with Controller %> + <% with $Controller %> <% include CMSBreadcrumbs %> <% end_with %>
- <% if Fields.hasTabset %> - <% with Fields.fieldByName('Root') %> + <% if $Fields.hasTabset %> + <% with $Fields.fieldByName('Root') %>
    - <% loop Tabs %> - class="$extraClass"<% end_if %>>$Title + <% loop $Tabs %> + class="$extraClass"<% end_if %>>$Title <% end_loop %>
@@ -23,20 +23,20 @@
- <% with Controller %> + <% with $Controller %> $EditFormTools <% end_with %>
- <% if Message %> + <% if $Message %>

$Message

<% else %> <% end_if %>
- <% if Legend %>$Legend<% end_if %> - <% loop Fields %> + <% if $Legend %>$Legend<% end_if %> + <% loop $Fields %> $FieldHolder <% end_loop %>
@@ -44,12 +44,12 @@
- <% if Actions %> + <% if $Actions %>
- <% loop Actions %> + <% loop $Actions %> $Field <% end_loop %> - <% if Controller.LinkPreview %> + <% if $Controller.LinkPreview %> <% _t('LeftAndMain.PreviewButton', 'Preview') %> » @@ -57,6 +57,6 @@
<% end_if %>
-<% if IncludeFormTag %> +<% if $IncludeFormTag %>
<% end_if %> diff --git a/admin/templates/Includes/LeftAndMain_Menu.ss b/admin/templates/Includes/LeftAndMain_Menu.ss index ce11e587e..1f1da0c03 100644 --- a/admin/templates/Includes/LeftAndMain_Menu.ss +++ b/admin/templates/Includes/LeftAndMain_Menu.ss @@ -2,18 +2,18 @@
$Field
- <% if Title %>

$Title

<% end_if %> + <% if $Title %>

$Title

<% end_if %>
\$DefineFragment(toolbar-header-right)
\$DefineFragment(toolbar-header-left)
$Valueclass="ss-gridfield-{$FirstLast}"<% end_if %>>$Value
- <% loop Header %><% end_loop %> + <% loop $Header %><% end_loop %> - <% if ItemRows %> - <% loop ItemRows %> - <% loop ItemRow %><% end_loop %> + <% if $ItemRows %> + <% loop $ItemRows %> + <% loop $ItemRow %><% end_loop %> <% end_loop %> <% else %> diff --git a/templates/Includes/HtmlEditorField_viewfile.ss b/templates/Includes/HtmlEditorField_viewfile.ss index a2bf9f7c2..997fa0e46 100644 --- a/templates/Includes/HtmlEditorField_viewfile.ss +++ b/templates/Includes/HtmlEditorField_viewfile.ss @@ -38,10 +38,10 @@ - <% if Info %>
$Info
<% end_if %> + <% if $Info %>
$Info
<% end_if %>
- <% loop Fields %> + <% loop $Fields %> $FieldHolder <% end_loop %>
diff --git a/templates/Includes/UploadField_FileButtons.ss b/templates/Includes/UploadField_FileButtons.ss index 1f0e920c7..1c14f0823 100644 --- a/templates/Includes/UploadField_FileButtons.ss +++ b/templates/Includes/UploadField_FileButtons.ss @@ -1,4 +1,4 @@ -<% if canEdit %> +<% if $canEdit %> -<% if canDelete %> +<% if $canDelete %> <% end_if %> -<% if UploadField.canAttachExisting %> +<% if $UploadField.canAttachExisting %> <% end_if %> diff --git a/templates/RSSFeed.ss b/templates/RSSFeed.ss index 45bcab748..f4bc1ff97 100644 --- a/templates/RSSFeed.ss +++ b/templates/RSSFeed.ss @@ -6,14 +6,14 @@ $Description.XML - <% loop Entries %> + <% loop $Entries %> $Title.XML $AbsoluteLink - <% if Description %>$Description.AbsoluteLinks.XML<% end_if %> - <% if Date %>$Date.Rfc822 + <% if $Description %>$Description.AbsoluteLinks.XML<% end_if %> + <% if $Date %>$Date.Rfc822 <% else %>$Created.Rfc822<% end_if %> - <% if Author %>$Author.XML<% end_if %> + <% if $Author %>$Author.XML<% end_if %> $AbsoluteLink <% end_loop %> diff --git a/templates/RightLabelledFieldHolder.ss b/templates/RightLabelledFieldHolder.ss index f20e1ccaf..574ab26d4 100644 --- a/templates/RightLabelledFieldHolder.ss +++ b/templates/RightLabelledFieldHolder.ss @@ -1,5 +1,5 @@

$Field - <% if Message %>$Message<% end_if %> -

\ No newline at end of file + <% if $Message %>$Message<% end_if %> +

diff --git a/templates/SearchForm.ss b/templates/SearchForm.ss index c6a525d1e..8b4c9c148 100644 --- a/templates/SearchForm.ss +++ b/templates/SearchForm.ss @@ -1,15 +1,15 @@
- <% if Message %> + <% if $Message %>

$Message

<% else %> <% end_if %>
- <% loop Fields %> - $FieldHolder + <% loop $Fields %> + $FieldHolder <% end_loop %> - <% loop Actions %> - $Field + <% loop $Actions %> + $Field <% end_loop %>
diff --git a/templates/UploadField.ss b/templates/UploadField.ss index 9f9af1e87..ecf70ee30 100644 --- a/templates/UploadField.ss +++ b/templates/UploadField.ss @@ -13,7 +13,7 @@
- <% if Top.isActive %> + <% if $Top.isActive %> $UploadFieldFileButtons <% end_if %>
@@ -25,7 +25,7 @@ <% end_loop %> <% end_if %> -<% if canUpload || canAttachExisting %> +<% if $canUpload || $canAttachExisting %>
<% if canUpload %>
@@ -43,21 +43,21 @@ <% else %> <% _t('UploadField.ATTACHFILE', 'Attach a file') %> <% end_if %> - <% if canPreviewFolder %> + <% if $canPreviewFolder %> (<%t UploadField.UPLOADSINTO 'saves into /{path}' path=$FolderName %>) <% end_if %> - <% if canUpload %> + <% if $canUpload %> <% end_if %> - <% if canAttachExisting %> + <% if $canAttachExisting %> <% end_if %> - <% if canUpload %> + <% if $canUpload %> <% if not $autoUpload %> <% end_if %> diff --git a/templates/forms/CheckboxField_holder.ss b/templates/forms/CheckboxField_holder.ss index 7798acc6e..b70a52e6a 100644 --- a/templates/forms/CheckboxField_holder.ss +++ b/templates/forms/CheckboxField_holder.ss @@ -1,6 +1,6 @@
$Field - <% if Message %>$Message<% end_if %> - <% if Description %>$Description<% end_if %> + <% if $Message %>$Message<% end_if %> + <% if $Description %>$Description<% end_if %>
diff --git a/templates/forms/CheckboxField_holder_small.ss b/templates/forms/CheckboxField_holder_small.ss index 511dbb1be..a4f8d4423 100644 --- a/templates/forms/CheckboxField_holder_small.ss +++ b/templates/forms/CheckboxField_holder_small.ss @@ -1,5 +1,5 @@ $Field <% if $Title %> - -<% end_if %> \ No newline at end of file + +<% end_if %> diff --git a/templates/forms/CheckboxSetField.ss b/templates/forms/CheckboxSetField.ss index cb6a3bb45..49c298da0 100644 --- a/templates/forms/CheckboxSetField.ss +++ b/templates/forms/CheckboxSetField.ss @@ -1,8 +1,8 @@
    - <% if Options.Count %> - <% loop Options %> + <% if $Options.Count %> + <% loop $Options %>
  • - checked="checked"<% end_if %><% if isDisabled %> disabled="disabled"<% end_if %> /> + checked="checked"<% end_if %><% if $isDisabled %> disabled="disabled"<% end_if %> />
    • <% end_loop %> diff --git a/templates/forms/CompositeField.ss b/templates/forms/CompositeField.ss index 452f1c979..9ab9c820e 100644 --- a/templates/forms/CompositeField.ss +++ b/templates/forms/CompositeField.ss @@ -3,8 +3,8 @@ $Legend <% end_if %> - <% loop FieldList %> - <% if ColumnCount %> + <% loop $FieldList %> + <% if $ColumnCount %>
    $Field
    @@ -12,4 +12,4 @@ $Field <% end_if %> <% end_loop %> - \ No newline at end of file + diff --git a/templates/forms/CompositeField_holder.ss b/templates/forms/CompositeField_holder.ss index 83f015f0b..bb3042350 100644 --- a/templates/forms/CompositeField_holder.ss +++ b/templates/forms/CompositeField_holder.ss @@ -3,8 +3,8 @@ $Legend <% end_if %> - <% loop FieldList %> - <% if ColumnCount %> + <% loop $FieldList %> + <% if $ColumnCount %>
    $FieldHolder
    @@ -13,5 +13,5 @@ <% end_if %> <% end_loop %> - <% if Description %>$Description<% end_if %> - \ No newline at end of file + <% if $Description %>$Description<% end_if %> + diff --git a/templates/forms/DropdownField.ss b/templates/forms/DropdownField.ss index 00d654e82..5d5c24d04 100644 --- a/templates/forms/DropdownField.ss +++ b/templates/forms/DropdownField.ss @@ -1,5 +1,5 @@ diff --git a/templates/forms/FieldGroup.ss b/templates/forms/FieldGroup.ss index 21fd1aa43..c9b6eb94a 100644 --- a/templates/forms/FieldGroup.ss +++ b/templates/forms/FieldGroup.ss @@ -1,7 +1,7 @@ -
    id="$ID"<% end_if %>> - <% loop FieldList %> +
    id="$ID"<% end_if %>> + <% loop $FieldList %>
    $SmallFieldHolder
    <% end_loop %> -
    \ No newline at end of file +
    diff --git a/templates/forms/FieldGroup_DefaultFieldHolder.ss b/templates/forms/FieldGroup_DefaultFieldHolder.ss index cb9478322..5a1558e69 100644 --- a/templates/forms/FieldGroup_DefaultFieldHolder.ss +++ b/templates/forms/FieldGroup_DefaultFieldHolder.ss @@ -1,7 +1,7 @@ -
    id="$ID"<% end_if %>> - <% loop FieldList %> +
    id="$ID"<% end_if %>> + <% loop $FieldList %>
    $FieldHolder
    <% end_loop %> -
    \ No newline at end of file +
    diff --git a/templates/forms/FieldGroup_holder.ss b/templates/forms/FieldGroup_holder.ss index 423baae1c..95ab1a153 100644 --- a/templates/forms/FieldGroup_holder.ss +++ b/templates/forms/FieldGroup_holder.ss @@ -1,13 +1,13 @@ -
    id="$Name"<% end_if %> class="field $Type $extraClass"> - <% if Title %><% end_if %> +
    id="$Name"<% end_if %> class="field $Type $extraClass"> + <% if $Title %><% end_if %>
    - <% loop FieldList %> + <% loop $FieldList %>
    $SmallFieldHolder
    <% end_loop %>
    - <% if RightTitle %><% end_if %> - <% if Message %>$Message<% end_if %> -
    \ No newline at end of file + <% if $RightTitle %><% end_if %> + <% if $Message %>$Message<% end_if %> +
    diff --git a/templates/forms/FormAction.ss b/templates/forms/FormAction.ss index fb5cf21ee..16e61fb1c 100644 --- a/templates/forms/FormAction.ss +++ b/templates/forms/FormAction.ss @@ -1,7 +1,7 @@ -<% if UseButtonTag %> +<% if $UseButtonTag %> <% else %> -<% end_if %> \ No newline at end of file +<% end_if %> diff --git a/templates/forms/FormField.ss b/templates/forms/FormField.ss index 73a06489b..8c763daf1 100644 --- a/templates/forms/FormField.ss +++ b/templates/forms/FormField.ss @@ -1,5 +1,5 @@ -<% if isReadonly %> - class="$extraClass"<% end_if %>> +<% if $isReadonly %> + class="$extraClass"<% end_if %>> $Value <% else %> diff --git a/templates/forms/FormField_holder.ss b/templates/forms/FormField_holder.ss index fa23b8595..ef1f917d9 100644 --- a/templates/forms/FormField_holder.ss +++ b/templates/forms/FormField_holder.ss @@ -1,9 +1,9 @@ -
    - <% if Title %><% end_if %> +
    + <% if $Title %><% end_if %>
    $Field
    - <% if RightTitle %><% end_if %> - <% if Message %>$Message<% end_if %> - <% if Description %>$Description<% end_if %> -
    \ No newline at end of file + <% if $RightTitle %><% end_if %> + <% if $Message %>$Message<% end_if %> + <% if $Description %>$Description<% end_if %> +
    diff --git a/templates/forms/FormField_holder_small.ss b/templates/forms/FormField_holder_small.ss index 4360afac5..101a190c5 100644 --- a/templates/forms/FormField_holder_small.ss +++ b/templates/forms/FormField_holder_small.ss @@ -1,11 +1,11 @@
    <% if $RightTitle %> - + <% else_if $LeftTitle %> - + <% else_if $Title %> - + <% end_if %> $Field -
    \ No newline at end of file +
diff --git a/templates/forms/OptionsetField.ss b/templates/forms/OptionsetField.ss index 445e3cd0b..bb8818a6f 100644 --- a/templates/forms/OptionsetField.ss +++ b/templates/forms/OptionsetField.ss @@ -1,7 +1,7 @@
    - <% loop Options %> + <% loop $Options %>
  • - checked<% end_if %><% if isDisabled %> disabled<% end_if %> /> + checked<% end_if %><% if $isDisabled %> disabled<% end_if %> />
    • <% end_loop %> diff --git a/templates/forms/SelectionGroup.ss b/templates/forms/SelectionGroup.ss index dccec5d6f..14737897c 100644 --- a/templates/forms/SelectionGroup.ss +++ b/templates/forms/SelectionGroup.ss @@ -1,14 +1,20 @@ -<% if IsReadonly %> +<% if $IsReadonly %>
      - <% loop FieldSet %> - <% if Selected %> - - $RadioLabel - $FieldHolder - + <% loop $FieldSet %> + <% if $Selected %> + + $RadioLabel + $FieldHolder +
    <% end_if %> <% end_loop %> <% else %> -
      <% loop FieldSet %>{$RadioButton}{$RadioLabel}{$FieldHolder}<% end_loop %>
    +
      + <% loop $FieldSet %> + + {$RadioButton}{$RadioLabel}{$FieldHolder} + + <% end_loop %> +
    <% end_if %> diff --git a/templates/forms/TabSet.ss b/templates/forms/TabSet.ss index 9f1535037..3d748954f 100644 --- a/templates/forms/TabSet.ss +++ b/templates/forms/TabSet.ss @@ -1,16 +1,16 @@
      - <% loop Tabs %> + <% loop $Tabs %>
    • $Title
      • <% end_loop %>
    - <% loop Tabs %> - <% if Tabs %> + <% loop $Tabs %> + <% if $Tabs %> $FieldHolder <% else %>
    - <% loop Fields %> + <% loop $Fields %> $FieldHolder <% end_loop %>
    diff --git a/templates/forms/TreeDropdownField.ss b/templates/forms/TreeDropdownField.ss index 762f65555..016acee33 100644 --- a/templates/forms/TreeDropdownField.ss +++ b/templates/forms/TreeDropdownField.ss @@ -2,7 +2,7 @@ class="TreeDropdownField single<% if extraClass %> $extraClass<% end_if %><% if ShowSearch %> searchable<% end_if %>" data-url-tree="$Link(tree)" data-title="$TitleURLEncoded" - <% if Description %>title="$Description"<% end_if %> - <% if Metadata %>data-metadata="$Metadata"<% end_if %>> + <% if $Description %>title="$Description"<% end_if %> + <% if $Metadata %>data-metadata="$Metadata"<% end_if %>> -
    \ No newline at end of file +
From d4a1e6d294da5e51e174233f33f2a56828a3ff72 Mon Sep 17 00:00:00 2001 From: Ingo Schommer Date: Thu, 11 Jul 2013 10:29:21 +0200 Subject: [PATCH 11/16] BUG Prevent clickjacking in CMS and Security controllers (fixes #2215) --- admin/code/LeftAndMain.php | 3 +++ docs/en/topics/security.md | 27 +++++++++++++++++++++++++++ security/Security.php | 7 +++++++ 3 files changed, 37 insertions(+) diff --git a/admin/code/LeftAndMain.php b/admin/code/LeftAndMain.php index 40198e6c4..5e379f327 100644 --- a/admin/code/LeftAndMain.php +++ b/admin/code/LeftAndMain.php @@ -449,6 +449,9 @@ class LeftAndMain extends Controller implements PermissionProvider { $title = $this->Title(); if(!$response->getHeader('X-Controller')) $response->addHeader('X-Controller', $this->class); if(!$response->getHeader('X-Title')) $response->addHeader('X-Title', urlencode($title)); + + // Prevent clickjacking, see https://developer.mozilla.org/en-US/docs/HTTP/X-Frame-Options + $this->response->addHeader('X-Frame-Options', 'SAMEORIGIN'); return $response; } diff --git a/docs/en/topics/security.md b/docs/en/topics/security.md index 3e64d5b6c..812243f25 100644 --- a/docs/en/topics/security.md +++ b/docs/en/topics/security.md @@ -407,6 +407,8 @@ configuration and test fixtures). You should therefore block access to all yaml files (extension .yml) by default, and white list only yaml files you need to serve directly. +See [Apache](/installation/webserver) and [Nginx](/installation/nginx) installation documentation for details +specific to your web server See [Apache](/installation/webserver) and [Nginx](/installation/nginx) installation documentation for details specific to your web server ## Passwords @@ -437,6 +439,31 @@ In addition, you can tighten password security with the following configuration * `Member.lock_out_delay_mins`: Minutes of enforced lockout after incorrect password attempts. Only applies if `lock_out_after_incorrect_logins` is greater than 0. +## Clickjacking: Prevent iframe Inclusion + +"[Clickjacking](http://en.wikipedia.org/wiki/Clickjacking)" is a malicious technique +where a web user is tricked into clicking on hidden interface elements, which can +lead to the attacker gaining access to user data or taking control of the website behaviour. + +You can signal to browsers that the current response isn't allowed to be +included in HTML "frame" or "iframe" elements, and thereby prevent the most common +attack vector. This is done through a HTTP header, which is usually added in your +controller's `init()` method: + + :::php + class MyController extends Controller { + public function init() { + parent::init(); + + $this->response->addHeader('X-Frame-Options', 'SAMEORIGIN'); + } + } + + +This is a recommended option to secure any controller which displays +or submits sensitive user input, and is enabled by default in all CMS controllers, +as well as the login form. + ## Related * [http://silverstripe.org/security-releases/](http://silverstripe.org/security-releases/) diff --git a/security/Security.php b/security/Security.php index 498514d0b..cc08025ae 100644 --- a/security/Security.php +++ b/security/Security.php @@ -270,6 +270,13 @@ class Security extends Controller { return; } + public function init() { + parent::init(); + + // Prevent clickjacking, see https://developer.mozilla.org/en-US/docs/HTTP/X-Frame-Options + $this->response->addHeader('X-Frame-Options', 'SAMEORIGIN'); + } + /** * Get the login form to process according to the submitted data From 999fd3901cf6190f96c63f83978aeb3e09f45946 Mon Sep 17 00:00:00 2001 From: Stephen Shkardoon Date: Mon, 15 Jul 2013 16:50:41 +1200 Subject: [PATCH 12/16] FIX preg_replace_callback in emailer was broken Fixes bug introduced by 9deb11f9a0ee01477af38f2826b2a7583545a0b6 Email has no content This is what happens when we make commits without testing! --- email/Mailer.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/email/Mailer.php b/email/Mailer.php index c45fdffe8..cf2e55cfd 100644 --- a/email/Mailer.php +++ b/email/Mailer.php @@ -424,7 +424,7 @@ function encodeFileForEmail($file, $destFileName = false, $disposition = NULL, $ function QuotedPrintable_encode($quotprint) { $quotprint = (string)str_replace('\r\n',chr(13).chr(10),$quotprint); $quotprint = (string)str_replace('\n', chr(13).chr(10),$quotprint); - $quotprint = (string)preg_replace_callback("~([\x01-\x1F\x3D\x7F-\xFF])~e", function($matches) { + $quotprint = (string)preg_replace_callback("~([\x01-\x1F\x3D\x7F-\xFF])~", function($matches) { return sprintf('=%02X', ord($matches[1])); }, $quotprint); //$quotprint = (string)str_replace('\=0D=0A',"=0D=0A",$quotprint); From d4a6f7304e60c67e2ace461b2cbb7390edc67264 Mon Sep 17 00:00:00 2001 From: Mateusz Uzdowski Date: Thu, 18 Jul 2013 18:01:53 +1200 Subject: [PATCH 13/16] BUG First error should take precedence here. No further checks needed. --- security/Member.php | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/security/Member.php b/security/Member.php index 90afb5101..051c6798d 100644 --- a/security/Member.php +++ b/security/Member.php @@ -219,6 +219,9 @@ class Member extends DataObject implements TemplateGlobalProvider { public function checkPassword($password) { $result = $this->canLogIn(); + // Short-circuit the result upon failure, no further checks needed. + if (!$result->valid()) return $result; + if(empty($this->Password) && $this->exists()) { $result->error(_t('Member.NoPassword','There is no password on this member.')); return $result; @@ -442,7 +445,9 @@ class Member extends DataObject implements TemplateGlobalProvider { self::session_regenerate_id(); Session::set("loggedInAs", $member->ID); // This lets apache rules detect whether the user has logged in - if(Member::config()->login_marker_cookie) Cookie::set(Member::config()->login_marker_cookie, 1, 0, null, null, false, true); + if(Member::config()->login_marker_cookie) { + Cookie::set(Member::config()->login_marker_cookie, 1, 0, null, null, false, true); + } $generator = new RandomGenerator(); $token = $generator->randomToken('sha1'); @@ -730,7 +735,8 @@ class Member extends DataObject implements TemplateGlobalProvider { $encryption_details = Security::encrypt_password( $this->Password, // this is assumed to be cleartext $this->Salt, - ($this->PasswordEncryption) ? $this->PasswordEncryption : Security::config()->password_encryption_algorithm, + ($this->PasswordEncryption) ? + $this->PasswordEncryption : Security::config()->password_encryption_algorithm, $this ); From 1298d4a5bd927117f9893f32fd02a75ed10d623b Mon Sep 17 00:00:00 2001 From: Hamish Friedlander Date: Thu, 18 Jul 2013 17:09:21 +1200 Subject: [PATCH 14/16] FIX Prevent DOS by checking for env and admin on ?flush=1 (#1692) --- core/startup/ErrorControlChain.php | 121 ++++++++++++++++ core/startup/ParameterConfirmationToken.php | 113 +++++++++++++++ docs/en/changelogs/3.0.6.md | 29 +++- docs/en/reference/urlvariabletools.md | 4 +- docs/en/topics/caching.md | 26 ++++ docs/en/topics/index.md | 1 + main.php | 139 +++++++++++++------ tests/core/startup/ErrorControlChainTest.php | 90 ++++++++++++ 8 files changed, 479 insertions(+), 44 deletions(-) create mode 100644 core/startup/ErrorControlChain.php create mode 100644 core/startup/ParameterConfirmationToken.php create mode 100644 docs/en/topics/caching.md create mode 100644 tests/core/startup/ErrorControlChainTest.php diff --git a/core/startup/ErrorControlChain.php b/core/startup/ErrorControlChain.php new file mode 100644 index 000000000..9cd344ee0 --- /dev/null +++ b/core/startup/ErrorControlChain.php @@ -0,0 +1,121 @@ +then($callback1)->then($callback2)->then(true, $callback3)->execute(); + * + * WARNING: This class is experimental and designed specifically for use pre-startup in main.php + * It will likely be heavily refactored before the release of 3.2 + */ +class ErrorControlChain { + protected $error = false; + protected $steps = array(); + + protected $suppression = true; + + /** We can't unregister_shutdown_function, so this acts as a flag to enable handling */ + protected $handleFatalErrors = false; + + public function hasErrored() { + return $this->error; + } + + public function setErrored($error) { + $this->error = (bool)$error; + } + + public function setSuppression($suppression) { + $this->suppression = (bool)$suppression; + } + + /** + * Add this callback to the chain of callbacks to call along with the state + * that $error must be in this point in the chain for the callback to be called + * + * @param $callback - The callback to call + * @param $onErrorState - false if only call if no errors yet, true if only call if already errors, null for either + * @return $this + */ + public function then($callback, $onErrorState = false) { + $this->steps[] = array( + 'callback' => $callback, + 'onErrorState' => $onErrorState + ); + return $this; + } + + public function thenWhileGood($callback) { + return $this->then($callback, false); + } + + public function thenIfErrored($callback) { + return $this->then($callback, true); + } + + public function thenAlways($callback) { + return $this->then($callback, null); + } + + public function handleError() { + if ($this->suppression && defined('BASE_URL')) throw new Exception('Generic Error'); + else return false; + } + + protected function lastErrorWasFatal() { + $error = error_get_last(); + return $error && $error['type'] == 1; + } + + public function handleFatalError() { + if ($this->handleFatalErrors && $this->suppression && defined('BASE_URL')) { + if ($this->lastErrorWasFatal()) { + ob_clean(); + $this->error = true; + $this->step(); + } + } + } + + public function execute() { + set_error_handler(array($this, 'handleError'), error_reporting()); + register_shutdown_function(array($this, 'handleFatalError')); + $this->handleFatalErrors = true; + + $this->step(); + } + + protected function step() { + if ($this->steps) { + $step = array_shift($this->steps); + + if ($step['onErrorState'] === null || $step['onErrorState'] === $this->error) { + try { + call_user_func($step['callback'], $this); + } + catch (Exception $e) { + if ($this->suppression && defined('BASE_URL')) $this->error = true; + else throw $e; + } + } + + $this->step(); + } + else { + // Now clean up + $this->handleFatalErrors = false; + restore_error_handler(); + } + } +} diff --git a/core/startup/ParameterConfirmationToken.php b/core/startup/ParameterConfirmationToken.php new file mode 100644 index 000000000..cc47bc2bb --- /dev/null +++ b/core/startup/ParameterConfirmationToken.php @@ -0,0 +1,113 @@ +randomToken('md5'); + + // Store a file in the session save path (safer than /tmp, as open_basedir might limit that) + file_put_contents($this->pathForToken($token), $token); + + return $token; + } + + protected function checkToken($token) { + $file = $this->pathForToken($token); + $content = null; + + if (file_exists($file)) { + $content = file_get_contents($file); + unlink($file); + } + + return $content == $token; + } + + public function __construct($parameterName) { + // Store the parameter name + $this->parameterName = $parameterName; + // Store the parameter value + $this->parameter = isset($_GET[$parameterName]) ? $_GET[$parameterName] : null; + // Store the token + $this->token = isset($_GET[$parameterName.'token']) ? $_GET[$parameterName.'token'] : null; + + // If a token was provided, but isn't valid, just throw a 403 + if ($this->token && (!$this->checkToken($this->token))) { + header("HTTP/1.0 403 Forbidden", true, 403); + die; + } + } + + public function parameterProvided() { + return $this->parameter !== null; + } + + public function tokenProvided() { + return $this->token !== null; + } + + public function params() { + return array( + $this->parameterName => $this->parameter, + $this->parameterName.'token' => $this->genToken() + ); + } + + public function reloadWithToken() { + global $url; + + // Are we http or https? + $proto = 'http'; + + if(isset($_SERVER['HTTP_X_FORWARDED_PROTOCOL'])) { + if(strtolower($_SERVER['HTTP_X_FORWARDED_PROTOCOL']) == 'https') $proto = 'https'; + } + + if((!empty($_SERVER['HTTPS']) && $_SERVER['HTTPS'] != 'off')) $proto = 'https'; + if(isset($_SERVER['SSL'])) $proto = 'https'; + + // What's our host + $host = $_SERVER['HTTP_HOST']; + + // What's our GET params (ensuring they include the original parameter + a new token) + $params = array_merge($_GET, $this->params()); + unset($params['url']); + + // Join them all together into the original URL + $location = "$proto://" . $host . BASE_URL . $url . ($params ? '?'.http_build_query($params) : ''); + + // And redirect + header('location: '.$location, true, 302); + die; + } +} diff --git a/docs/en/changelogs/3.0.6.md b/docs/en/changelogs/3.0.6.md index 7e6448cf0..ba7e28704 100644 --- a/docs/en/changelogs/3.0.6.md +++ b/docs/en/changelogs/3.0.6.md @@ -1,5 +1,32 @@ # 3.0.6 (Not yet released) +## Overview + + * Security: Require ADMIN for `?flush=1` (stop denial of service attacks) + ([#1692](https://github.com/silverstripe/silverstripe-framework/issues/1692)) + +## Details + +### Security: Require ADMIN for ?flush=1 + +Flushing the various manifests (class, template, config) is performed through a GET +parameter (`flush=1`). Since this action requires more server resources than normal requests, +it can facilitate [denial-of-service attacks](https://en.wikipedia.org/wiki/Denial-of-service_attack). + +To prevent this, main.php now checks and only allows the flush parameter in the following cases: + + * The [environment](/topics/environment-management) is in "dev mode" + * A user is logged in with ADMIN permissions + * An error occurs during startup + +This applies to both `flush=1` and `flush=all` (technically we only check for the existence of any parameter value) +but only through web requests made through main.php - CLI requests, or any other request that goes through +a custom start up script will still process all flush requests as normal. + ## Upgrading - * If you have created your own composite database fields, then you shoulcd amend the setValue() to allow the passing of an object (usually DataObject) as well as an array. + * If you have created your own composite database fields, then you should amend the setValue() to allow the passing of + an object (usually DataObject) as well as an array. + + * If you have provided your own startup scripts (ones that include core/Core.php) that can be accessed via a web + request, you should ensure that you limit use of the flush parameter diff --git a/docs/en/reference/urlvariabletools.md b/docs/en/reference/urlvariabletools.md index 8424628f2..d715e3808 100644 --- a/docs/en/reference/urlvariabletools.md +++ b/docs/en/reference/urlvariabletools.md @@ -17,10 +17,8 @@ Append the option and corresponding value to your URL in your browser's address | URL Variable | | Values | | Description | | ------------ | | ------ | | ----------- | - | flush | | 1,all | | This will clear out all cached information about the page. This is used frequently during development - for example, when adding new PHP or SS files. See below for value descriptions. | + | flush=1 | | 1 | | Clears out all caches. Used mainly during development, e.g. when adding new classes or templates. Requires "dev" mode or ADMIN login | | showtemplate | | 1 | | Show the compiled version of all the templates used, including line numbers. Good when you have a syntax error in a template. Cannot be used on a Live site without **isDev**. **flush** can be used with the following values: | - | ?flush=1 | | | | Flushes the current page and included templates | - | ?flush=all | | | | Flushes the entire template cache | ## General Testing diff --git a/docs/en/topics/caching.md b/docs/en/topics/caching.md new file mode 100644 index 000000000..61a6a4225 --- /dev/null +++ b/docs/en/topics/caching.md @@ -0,0 +1,26 @@ +# Caching + +## Built-In Caches + +The framework uses caches to store infrequently changing values. +By default, the storage mechanism is simply the filesystem, although +other cache backends can be configured. All caches use the `[api:SS_Cache]` API. + +The most common caches are manifests of various resources: + + * PHP class locations (`[api:SS_ClassManifest]`) + * Template file locations and compiled templates (`[api:SS_TemplateManifest]`) + * Configuration settings from YAML files (`[api:SS_ConfigManifest]`) + * Language files (`[api:i18n]`) + +Flushing the various manifests is performed through a GET +parameter (`flush=1`). Since this action requires more server resources than normal requests, +executing the action is limited to the following cases when performed via a web request: + + * The [environment](/topics/environment-management) is in "dev mode" + * A user is logged in with ADMIN permissions + * An error occurs during startup + +## Custom Caches + +See `[api:SS_Cache]`. \ No newline at end of file diff --git a/docs/en/topics/index.md b/docs/en/topics/index.md index 1b31c3521..91a468c4b 100644 --- a/docs/en/topics/index.md +++ b/docs/en/topics/index.md @@ -4,6 +4,7 @@ This section provides an overview on how things fit together, the "conceptual gl It is where most documentation should live, and is the natural "second step" after finishing the tutorials. * [Access Control and Page Security](access-control): Restricting access and setting up permissions on your website + * [Caching](caching): Explains built-in caches for classes, config and templates. How to use your own caches. * [Command line Usage](commandline): Calling controllers via the command line interface using `sake` * [Configuring your website](configuration): How to configure the `_config.php` file * [Controller](controller): The intermediate layer between your templates and the data model diff --git a/main.php b/main.php index b81ef254f..fb76d4872 100644 --- a/main.php +++ b/main.php @@ -59,44 +59,108 @@ if (version_compare(phpversion(), '5.3.2', '<')) { /** * Include SilverStripe's core code */ -require_once('core/Core.php'); +require_once('core/startup/ErrorControlChain.php'); +require_once('core/startup/ParameterConfirmationToken.php'); -// IIS will sometimes generate this. -if(!empty($_SERVER['HTTP_X_ORIGINAL_URL'])) { - $_SERVER['REQUEST_URI'] = $_SERVER['HTTP_X_ORIGINAL_URL']; -} +$chain = new ErrorControlChain(); +$token = new ParameterConfirmationToken('flush'); -// Apache rewrite rules use this -if (isset($_GET['url'])) { - $url = $_GET['url']; - // IIS includes get variables in url - $i = strpos($url, '?'); - if($i !== false) { - $url = substr($url, 0, $i); - } - -// Lighttpd uses this -} else { - if(strpos($_SERVER['REQUEST_URI'],'?') !== false) { - list($url, $query) = explode('?', $_SERVER['REQUEST_URI'], 2); - parse_str($query, $_GET); - if ($_GET) $_REQUEST = array_merge((array)$_REQUEST, (array)$_GET); - } else { - $url = $_SERVER["REQUEST_URI"]; - } -} +$chain + // First, if $_GET['flush'] was set, but no valid token, suppress the flush + ->then(function($chain) use ($token){ + if (isset($_GET['flush']) && !$token->tokenProvided()) { + unset($_GET['flush']); + } + else { + $chain->setSuppression(false); + } + }) + // Then load in core + ->then(function(){ + require_once('core/Core.php'); + }) + // Then build the URL (even if Core didn't load beyond setting BASE_URL) + ->thenAlways(function(){ + global $url; -// Remove base folders from the URL if webroot is hosted in a subfolder -if (substr(strtolower($url), 0, strlen(BASE_URL)) == strtolower(BASE_URL)) $url = substr($url, strlen(BASE_URL)); + // IIS will sometimes generate this. + if(!empty($_SERVER['HTTP_X_ORIGINAL_URL'])) { + $_SERVER['REQUEST_URI'] = $_SERVER['HTTP_X_ORIGINAL_URL']; + } -if (isset($_GET['debug_profile'])) { - Profiler::init(); - Profiler::mark('all_execution'); - Profiler::mark('main.php init'); -} + // Apache rewrite rules use this + if (isset($_GET['url'])) { + $url = $_GET['url']; + // IIS includes get variables in url + $i = strpos($url, '?'); + if($i !== false) { + $url = substr($url, 0, $i); + } -// Connect to database -require_once('model/DB.php'); + // Lighttpd uses this + } else { + if(strpos($_SERVER['REQUEST_URI'],'?') !== false) { + list($url, $query) = explode('?', $_SERVER['REQUEST_URI'], 2); + parse_str($query, $_GET); + if ($_GET) $_REQUEST = array_merge((array)$_REQUEST, (array)$_GET); + } else { + $url = $_SERVER["REQUEST_URI"]; + } + } + + // Remove base folders from the URL if webroot is hosted in a subfolder + if (substr(strtolower($url), 0, strlen(BASE_URL)) == strtolower(BASE_URL)) $url = substr($url, strlen(BASE_URL)); + }) + // Then start up the database + ->then(function(){ + if (isset($_GET['debug_profile'])) { + Profiler::init(); + Profiler::mark('all_execution'); + Profiler::mark('main.php init'); + } + + require_once('model/DB.php'); + global $databaseConfig; + + if (isset($_GET['debug_profile'])) Profiler::mark('DB::connect'); + if ($databaseConfig) DB::connect($databaseConfig); + if (isset($_GET['debug_profile'])) Profiler::unmark('DB::connect'); + }) + // Then if a flush was requested, redirect to it + ->then(function($chain) use ($token){ + if ($token->parameterProvided() && !$token->tokenProvided()) { + // First, check if we're in dev mode, or the database doesn't have any security data + $canFlush = Director::isDev() || !Security::database_is_ready(); + + // Otherwise, we start up the session if needed, then check for admin + if (!$canFlush) { + if(!isset($_SESSION) && (isset($_COOKIE[session_name()]) || isset($_REQUEST[session_name()]))) { + Session::start(); + } + + if (Permission::check('ADMIN')) { + $canFlush = true; + } + else { + $loginPage = Director::absoluteURL(Config::inst()->get('Security', 'login_url')); + $loginPage .= "?BackURL=" . urlencode($_SERVER['REQUEST_URI']); + + header('location: '.$loginPage, true, 302); + die; + } + } + + // And if we can flush, reload with an authority token + if ($canFlush) $token->reloadWithToken(); + } + }) + // Finally if a flush was requested but there was an error while figuring out if it's allowed, do it anyway + ->thenIfErrored(function() use ($token){ + if ($token->parameterProvided() && !$token->tokenProvided()) { + $token->reloadWithToken(); + } + }) + ->execute(); // Redirect to the installer if no database is selected if(!isset($databaseConfig) || !isset($databaseConfig['database']) || !$databaseConfig['database']) { @@ -105,22 +169,17 @@ if(!isset($databaseConfig) || !isset($databaseConfig['database']) || !$databaseC } $s = (isset($_SERVER['SSL']) || (isset($_SERVER['HTTPS']) && $_SERVER['HTTPS'] != 'off')) ? 's' : ''; $installURL = "http$s://" . $_SERVER['HTTP_HOST'] . BASE_URL . '/install.php'; - + // The above dirname() will equate to "\" on Windows when installing directly from http://localhost (not using // a sub-directory), this really messes things up in some browsers. Let's get rid of the backslashes $installURL = str_replace('\\', '', $installURL); - + header("Location: $installURL"); die(); } -if (isset($_GET['debug_profile'])) Profiler::mark('DB::connect'); -DB::connect($databaseConfig); -if (isset($_GET['debug_profile'])) Profiler::unmark('DB::connect'); - if (isset($_GET['debug_profile'])) Profiler::unmark('main.php init'); - // Direct away - this is the "main" function, that hands control to the appropriate controller DataModel::set_inst(new DataModel()); Director::direct($url, DataModel::inst()); diff --git a/tests/core/startup/ErrorControlChainTest.php b/tests/core/startup/ErrorControlChainTest.php new file mode 100644 index 000000000..e9c2c8435 --- /dev/null +++ b/tests/core/startup/ErrorControlChainTest.php @@ -0,0 +1,90 @@ +then(function(){ + user_error('This error should be suppressed', E_USER_ERROR); + }) + ->execute(); + + $this->assertTrue($chain->hasErrored()); + } + + function testMultipleErrorSuppression() { + $chain = new ErrorControlChain(); + + $chain + ->then(function(){ + user_error('This error should be suppressed', E_USER_ERROR); + }) + ->thenAlways(function(){ + user_error('This error should also be suppressed', E_USER_ERROR); + }) + ->execute(); + + $this->assertTrue($chain->hasErrored()); + } + + function testExceptionSuppression() { + $chain = new ErrorControlChain(); + + $chain + ->then(function(){ + throw new Exception('This exception should be suppressed'); + }) + ->execute(); + + $this->assertTrue($chain->hasErrored()); + } + + function testMultipleExceptionSuppression() { + $chain = new ErrorControlChain(); + + $chain + ->then(function(){ + throw new Exception('This exception should be suppressed'); + }) + ->thenAlways(function(){ + throw new Exception('This exception should also be suppressed'); + }) + ->execute(); + + $this->assertTrue($chain->hasErrored()); + } + + function testErrorControl() { + $preError = $postError = array('then' => false, 'thenIfErrored' => false, 'thenAlways' => false); + + $chain = new ErrorControlChain(); + + $chain + ->then(function() use (&$preError) { $preError['then'] = true; }) + ->thenIfErrored(function() use (&$preError) { $preError['thenIfErrored'] = true; }) + ->thenAlways(function() use (&$preError) { $preError['thenAlways'] = true; }) + + ->then(function(){ user_error('An error', E_USER_ERROR); }) + + ->then(function() use (&$postError) { $postError['then'] = true; }) + ->thenIfErrored(function() use (&$postError) { $postError['thenIfErrored'] = true; }) + ->thenAlways(function() use (&$postError) { $postError['thenAlways'] = true; }) + + ->execute(); + + $this->assertEquals( + array('then' => true, 'thenIfErrored' => false, 'thenAlways' => true), + $preError, + 'Then and thenAlways callbacks called before error, thenIfErrored callback not called' + ); + + $this->assertEquals( + array('then' => false, 'thenIfErrored' => true, 'thenAlways' => true), + $postError, + 'thenIfErrored and thenAlways callbacks called after error, then callback not called' + ); + } + +} \ No newline at end of file From 036c36a7ddf2ac25906b9fc62c77a63a1ce6be0f Mon Sep 17 00:00:00 2001 From: Hamish Friedlander Date: Fri, 19 Jul 2013 14:33:56 +1200 Subject: [PATCH 15/16] FIX: Have ParameterConfirmationToken work regardless of include path --- core/startup/ParameterConfirmationToken.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/startup/ParameterConfirmationToken.php b/core/startup/ParameterConfirmationToken.php index cc47bc2bb..acebc921c 100644 --- a/core/startup/ParameterConfirmationToken.php +++ b/core/startup/ParameterConfirmationToken.php @@ -23,7 +23,7 @@ class ParameterConfirmationToken { $basepath = rtrim(dirname(dirname(dirname(dirname(__FILE__)))), DIRECTORY_SEPARATOR); } - require_once('core/TempPath.php'); + require_once(dirname(dirname(__FILE__)).'/TempPath.php'); $tempfolder = getTempFolder($basepath ? $basepath : DIRECTORY_SEPARATOR); return $tempfolder.'/token_'.preg_replace('/[^a-z0-9]+/', '', $token); @@ -31,7 +31,7 @@ class ParameterConfirmationToken { protected function genToken() { // Generate a new random token (as random as possible) - require_once('security/RandomGenerator.php'); + require_once(dirname(dirname(dirname(__FILE__))).'/security/RandomGenerator.php'); $rg = new RandomGenerator(); $token = $rg->randomToken('md5'); From a312cd08e12daa55015f75d8642a621b2b460723 Mon Sep 17 00:00:00 2001 From: Hamish Friedlander Date: Fri, 19 Jul 2013 14:47:05 +1200 Subject: [PATCH 16/16] FIX: Ignore invalid tokens instead of throwing 403 --- core/startup/ParameterConfirmationToken.php | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/core/startup/ParameterConfirmationToken.php b/core/startup/ParameterConfirmationToken.php index acebc921c..e0ccf9d87 100644 --- a/core/startup/ParameterConfirmationToken.php +++ b/core/startup/ParameterConfirmationToken.php @@ -61,11 +61,8 @@ class ParameterConfirmationToken { // Store the token $this->token = isset($_GET[$parameterName.'token']) ? $_GET[$parameterName.'token'] : null; - // If a token was provided, but isn't valid, just throw a 403 - if ($this->token && (!$this->checkToken($this->token))) { - header("HTTP/1.0 403 Forbidden", true, 403); - die; - } + // If a token was provided, but isn't valid, ignore it + if ($this->token && (!$this->checkToken($this->token))) $this->token = null; } public function parameterProvided() {
$CellString
$CellString
$CellString
$CellString