We've enabled Travis support for the 2.x line, but we should add it for 1.x as well.

Currently 1.x pull requests will fail their status checks, which is potentially misleading.

RichLinksExtension adds title="Open external link" to any external links. We've had multiple accessibility experts recommend to remove title attributes from links. More information https://developer.paciellogroup.com/blog/2013/01/using-the-html-title-attribute-updated/

The instructions here https://www.cwp.govt.nz/developer-docs/en/2/how_tos/basic_auth/ for enabling basic auth on prod don't appear to be working correctly

---
Name: mysitesecuritylive
After: '#cwpsecuritylive'
---
SilverStripe\Security\BasicAuth:
  entire_site_protected: true

Non admin users (e.g. content author) were unable to get past the basic auth pop-up after entering their credentials and https://dash.cwp.govt.nz/naut/project/mystack/environment/prod/letmein was not working either

In ss3 admin a user would get alerted if alt text wasn't defined. This functionality isn't working with ss4. Is this likely to be implemented? I see the code is still in the repo but is using entwine which I assume no longer works with assets admin.

Alert message

{{ Image name }} has no alt text which is required for non-decorative images.

Are you sure you want to update?

https://www.nzism.gcsb.govt.nz/pdf/index/1802

As part of Suspension of Access, 16.1.29.C.01. state that agencies must have a system administrator reset locked accounts; which conflicts with the automatic lock out expiry time set by lock_out_delay_mins. Reading NZISM and it would seem that once an account is locked, only an administrator can unlock it, rather than the system automatically unlocking it.

This control applies to systems Confidential, Secret, Top Secret; Compliance which I would assume includes CWP given it's rated to In Confidence (equals Confidential?)

Related PR

• #78

As per @sminnee's comment

We’d also want to think about the UX for if someone logs in with a no-longer-compliant password. Do we force a reset?

We currently do not force a reset (to my knowledge). The flow could be evaluated on submission of the password before hashing, setting a flag to update iff (if and only if) that should lead to a successful logging-in.

I worry that this may appear to a semi-savvy user that the password is not stored securely ("how would they know what my password is to say that?"), so I think there would be some communication with whatever method this is communicated through to the user.

@clarkepaul @newleeland may be interested in this flow.

Description

By default MFA hashes backup codes with password_hash which will use either bcrypt or argon2i/d for hashing. NZISM specifies that password hashes should be computed with the key derivation function pbkdf2. This is actually (very slightly) worse than what's provided by MFA out of the box.

Acceptance Criteria

• Backup codes generated by the MFA module in CWP are hashed with an NZISM compliant hashing algo (pbkdf2 at time of writing) using unique salts per code.

Using dev-master (as at d980d7f049b9b8) in SilverStripe test mode (deployed to SilverStripe Platform UAT) when attempting to login to /admin the following exception is thrown:

Uncaught Exception BadMethodCallException: "No session available for this HTTPRequest"
at /var/www/mysite/releases/86b5b06b2217d65ec559d90e8e1280e2b2b86039/vendor/silverstripe/framework/src/Control/HTTPRequest.php
line 891

This results in an inability to even see the admin login screen, with a 500 error displaying instead.

I traced this back to Extension/CwpControllerExtension.php on line 124 where the call to getSession() in $this->owner->getRequest()->getSession()->get('AutoLoginHash') fails.

To "prove" this is the rogue logic, I removed this else if condition, redeployed and I could once again see the admin screen.

The "owner" is the current Controller which obviously (should) have a valid HTTPRequest object, but I haven't dug any deeper. Over to you @robbieaverill !?

Copying from the issue to implement this in CWP 2:

Blowfish is the default hashing algo for SilverStripe. It is still considered a safe hashing algo (https://en.wikipedia.org/wiki/Blowfish_(cipher)), but is not on the list of approved cryptographic algorithms in the NZISM: https://www.nzism.gcsb.govt.nz/ism-document/#2106. We should set SHA-512 as a new default. Since the system tracks the algo per user, I believe we can do this without password resets. It kicks in either when an existing user changes their password, or when new users create a password. It should retain the cost parameter used in the blowfish logic.

Note that blowfish is a crypto algo which can be used for encryption, but can also form the basis for the bcrypt hashing algorithm - which is how we're using it. Meaning we're hashing passwords, not encrypting them - even though the APIs in SilverStripe are named misleadingly (Security::encrypt_password()).

To be clear, this is not a security vulnerability, but rather a compliance issue.

The current help link in CWP is configured to point to cwp.govt.nz instead of docs.silverstripe.org: https://github.com/silverstripe/cwp-installer/blob/master/app/_config/cwpadmin.yml

With silverstripe/silverstripe-admin#615 (comment) we'll need to add new config to change this, for SilverStripe 4.3.x-dev (CWP 2.2)

When running /dev/tasks/Solr_Configure on CWP 1.9.1, I get the following notice in graylog:

PHP Notice: Undefined index: remotepath in /sites/mysite-uat/releases/2019051234/cwp-core/code/CwpSolrConfigStore.php on line 18

As far as I can tell, this does not actually affect the functionality of solr, it just creates an unexplained bad looking thing in Graylog which isn't good for confidence when doing a go-live

I've replicated this on 2 CWP sites on UAT

Both sites appear to be correctly using the cwp-4 solr configuration

I think this happens because Cwpsolr.php -> options_for_cwp() -> 'indexstore' does not contain the key 'remotepath'

Fulltext functionality is intended to be optional to the site's requirements, however is currently an imposed requirement. While it is not necessary to be used, it must be installed in order for a dev/build (or any other flushing activity) to succeed.

This issue is of two parts, one to separate out what is contained in this cwp-core repository, and the other to affect the cwp repository: silverstripe/cwp#21

Ideally there'll be e.g. cwp/cwp-search or suchlike that would house the adaptive functionality that enables the features provided by/for the platform, rather than essentially having an unlisted dependency for this module.

See @tractorcow's comment from #22 (comment):

I think that most of this extension can be shifted either to config, or to a custom middleware instead.

List of recommendations for refactor at https://github.com/silverstripe/cwp-core/compare/master...creative-commoners:pulls/2.0/cwp-recommendations?expand=1

A custom basic auth middleware would be needed to handle new logic:

• exclude based on configured IP
• authenticate based on change password token

But the rest is already codified in basic auth config variables.

Description

HTMLEditorField_Toolbar is no longer exist in CMS. Investigate replacement for HTMLEditorField_Toolbar.
See:

Migrated from GitLab

cc @jakedaleweb

This task occasionally hangs which causes jobs not to run. We can implement a time out through here:
https://github.com/silverstripe-australia/silverstripe-queuedjobs/blob/c5ed18700aa7ce20c704f6e996be7d848a273543/src/Services/QueuedJobService.php#L729

10 minutes might be good?

Blowfish is the default hashing algo for SilverStripe. It is still considered a safe hashing algo (https://en.wikipedia.org/wiki/Blowfish_(cipher)), but is not on the list of approved cryptographic algorithms in the NZISM: https://www.nzism.gcsb.govt.nz/ism-document/#2106. We should set SHA-512 as a new default. Since the system tracks the algo per user, I believe we can do this without password resets. It kicks in either when an existing user changes their password, or when new users create a password. It should retain the cost parameter used in the blowfish logic.

Note that blowfish is a crypto algo which can be used for encryption, but can also form the basis for the bcrypt hashing algorithm - which is how we're using it. Meaning we're hashing passwords, not encrypting them - even though the APIs in SilverStripe are named misleadingly (Security::encrypt_password()).

To be clear, this is not a security vulnerability, but rather a compliance issue.

/cc @robbieaverill @Firesphere

On deployment to AWS Solo (Yes, CWP shouldn't be on platform, but um it is as you'll know!)

[2017-12-22 14:52:31] <i-0e93708165c407d0f> deploy/release: Running dev/build
[2017-12-22 14:52:31] <i-0e93708165c407d0f> deploy/release:   1: ERROR [Emergency]: Uncaught TypeError: Argument 1 passed to SilverStripe\Security\BasicAuth::requireLogin() must be an instance of SilverStripe\Control\HTTPRequest, string given, called in /var/www/mysite/releases/af3c4769675adf1e8d1772f6ce90132fa0030181/vendor/cwp/cwp-core/code/extensions/CwpControllerExtension.php on line 156
[2017-12-22 14:52:31] <i-0e93708165c407d0f> deploy/release:   2: IN GET dev/build
[2017-12-22 14:52:31] <i-0e93708165c407d0f> deploy/release:   3: Line 80 in /var/www/mysite/releases/af3c4769675adf1e8d1772f6ce90132fa0030181/vendor/silverstripe/framework/src/Security/BasicAuth.php

We require 8 chars min length, but NZISM requires 10 since at least v2.4 (Nov 2015) - see archive.

Context: silverstripe/silverstripe-framework#8522. Tracking this separately for CWP, because we might need to handle this faster or in a different way. In open source, it's a matter of opinion. In CWP, it's a compliance issue. Note that agencies can choose to increase those requirements on their own instances, but is about setting compliant defaults.

https://www.nzism.gcsb.govt.nz/ism-document/#1858

Agencies SHOULD implement a password policy enforcing either:
a minimum password length o
f 16 characters with no complexity requirement; or
a minimum password length of ten characters, consisting of at least three of the following character sets:
lowercase characters (a-z);
uppercase characters (A-Z);
digits (0-9); and
punctuation and special characters.

This also aligns with OWASP guidance: https://www.owasp.org/index.php/Authentication_Cheat_Sheet#Password_Length

217ddec#diff-8a7315557cd9f672e36f7e8f0ce2c29e removed the password requirements from CWP, because they're now part of recipe-core: https://github.com/silverstripe/recipe-core/blob/4/app/_config.php#L8

Pull requests

• #53
• silverstripe/recipe-core#34
• silverstripe/silverstripe-framework#8587
• #59
• silverstripe/silverstripe-framework#8600
• silverstripe/recipe-core#35
• silverstripe/silverstripe-framework#8604

CWP Core recipe defines default CWP configuration for Solr. However this only applies to Revera based hosting. Cloud platforms do not have the same configuration meaning developers are needing to overwrite these settings in order to get Solr to work.

Silverstripe Cloud

• Requires manual implementation of QueuedJobs runner (not included by default now)
• Requires removal of default YML configuration for CWP
  E.g. Remove the following from config.yml in project

---
Except:
  constantdefined: CWP_ENVIRONMENT
---
CwpSolr:
  options:
    version: 'local-4'
---
Only:
  constantdefined: CWP_ENVIRONMENT
---
CwpSolr:
  options:
    version: 'cwp-4'
---

• Requires adding Solr configuraiton to codebase for Silverstripe Cloud - https://servicedesk.silverstripe.cloud/support/solutions/articles/75000012860-solr-search
• Running a Solr_Configure before making the above changes creates a bad index that requires Ops intervention to remove.

All these changes are required currently just to get Solr working with the CWP Recipe on Silverstripe Cloud.

Description

The following improvements need to be made:

• Rename CustomHtmlEditorField.php file to match class name CustomHtmlEditorFieldToolbar
• CwpAtomFeed - Improve documentation
• LoginAttemptNotifications update class description with more details.
• LoginAttemptNotifications fix message styles.

While adding https://github.com/silverleague/silverstripe-ideannotator to CWP2 project, class HTMLEditorField_Toolbar can't be found.

As per the .upgrade.yml, should the HTMLEditorField_Toolbar be replaced with SilverStripe\Admin\ModalController in the extensions.yml ?

https://github.com/silverstripe/cwp-core/blob/master/_config/extensions.yml#L20

The configuration of cwptextextraction (in

Furthermore the reference to the SS3 class FileTextCache_SSCache does not work anymore, because the class is now namespaced and can be found under SilverStripe\TextExtraction\Cache\FileTextCache\Cache. The config (in

The consequence of this is, that the default SilverStripe\TextExtraction\Cache\FileTextCache\Database is now used on CWP (as defined in https://github.com/silverstripe/silverstripe-textextraction/blob/5b967fd5d37f99ace30cdc9592be9729e41cf973/_config/config.yml#L7 ) instead of the previously used FileTextCache.

CWP 2.1, probably CWP 2.0 as well

The LoginAttemptNotifications extensions purpose is to show the CMS user when they log in the number of successful and/or unsuccessful login attempts that occurred since the previous time they logged into the CMS.

@chillu pointed out that it's not particularly useful to show CMS users that there were successful login attempts, seeing as the message looks like this (nobody will remember their IP and be able to distinguish a malicious login from it):

In the last 2 mins a successful login attempt to your account was registered. The attempt was made from 14.1.35.58.

In CWP 1.x this extension was disabled in 2013:

silverstripe/cwp@2c576c3#diff-8a7315557cd9f672e36f7e8f0ce2c29e

It was re-enabled during the CWP 2.x upgrade, possibly by mistake:

f46727f#diff-8a7315557cd9f672e36f7e8f0ce2c29eR125

It was then moved from _config.php into a YAML config file.

This issue is raised as a placeholder to either deprecated and remove this feature OR to investigate, fix and reimplement it so that it works correctly and doesn't show up every time a CMS user does something in the CMS.

Context: CWP kitchen sink recipe 2.1.x-dev on the CWP platform, not logged in as the default admin user.

Related bug fix for this functionality in core (UI fix): silverstripe/silverstripe-admin#568

When running Docvert on CWP, the Manifest fails to properly include this extension if Page class does not exist. This should be using BasePage or SiteTree as these classes exists within the cwp module that is required by this one.

E.g.

BasePage::add_extension('DocumentConverterDecorator');

For example, media and embed are not showing up:

CWP 2.0:

Vanilla SS4:

Use namespace SilverStripe\CWPCore CWP\Core.

Sub-tasks can be added separately or as items in a checklist on this issue.

Acceptance criteria

• Works on SS4
• Unit tests/CI builds pass
• Code is PSR-2 compliant
• Files are PSR-4 compliant
• Upgrader mapping file is valid
• Language files have been updated
• Transifex config .tx file has been removed from SS3 branches and merged up (SS)
• New translations pushed to Transifex
• Beta tag released

Sub tasks

• Bulk of upgrading work: pull request #6
• #5 Use middleware over request filters
• #4 Rename SolrSearchIndex to CwpSolrIndex
• #9 Update language files
• PR for TinyMCE plugins at #11
• Compatibility with subsites and cwp/cwp: #12
• Starter theme template path adjustments: silverstripe/cwp-starter-theme#13
• Move search functionality to a new module: #14

RequestFilter is deprecated, so we should replace this with a middleware

PR: #85

This was preventing MFA Account Resets on UAT environments.

AC's

• PR merged
• Merge-up to 2
• Other relevant DoD items completed

CWP 2.2.0-rc1

• Login to the CMS
• Expand the help links section
• Hover over "4.3"

Expected: Should say "CWP 2.2.0-rc1" (and "4.3" should be "2.2")

Actual: Says "SilverStripe 4.3.x-dev" and "4.3"

CWP 2.x

The default is to use CWP_IP_BYPASS_BASICAUTH as a string value for CwpBasicAuthMiddleware's whitelisted IPs.

If you want to merge in an array of custom IPs as well, you cannot do this because the string splitting on commas only works if the input so setWhitelistedIps() is a string.

It should work on all string values in an array as well.

Workaround is to use PHP logic in app/_config.php to achieve the same result.

cc @MasseyIsaako

https://github.com/silverstripe/cwp-core/blob/2/src/Control/CwpBasicAuthMiddleware.php#L82

$_SERVER[REMOTE_ADDR] should not be used directly, we've got HTTPRequest->getIP() for this purpose. Specifically, the HTTPRequest IP can be optionally overwritten with proxy IPs instead (through trusted proxy IPs and X-Forwarded-For headers). This works in other parts of framework.

When this code was written, it was reasonable to make assumptions about the infrastructure configuration. In the only valid WAF proxy available on CWP (Imperva), this handling of proxy IPs happens transparently in the infrastructure. This recipe might evolve beyond CWP though (e.g. porting code over, renaming the module), at which point it'll need to deal with other infrastructure proxy layers - and use the wrong information.

This isn't a security issue as such (doesn't allow bypass of the IP allowlist), instead it'll lock out everyone when infrastructure proxies are configured differently (IP allowlist will never match).

https://github.com/silverstripe/cwp-core/blob/master/_config/config.yml#L12 sets the session timeout for SilverStripe sessions to be one hour (3600 seconds), however PHP settings on CWP itself are not changed from the default value of 24mins (1440 seconds).

This means that while we set sessions and their cookies to expire after an hour, PHP is cleaning up these sessions after just 24mins, meaning that nobody gets to use a session with more idle time than 24mins.

It would make sense to set this value to 1440 to match PHP settings so it's not confusing to users.

Additionally, we could add documentation around how to increase this value, which consists of doing two things:

1. Overriding the YML config value with an updated value (e.g. 7200 for 2hrs)
2. Adding php_value session.gc_maxlifetime 7200 to .htaccess

PRs

• #47 - Config update
• silverstripe/cwp#148 - Docs

CWP provides the Graylog service for developers to record errors in. cwp-core previously in the 1.x line automatically logged messages into Graylog, but the 2.x line seems to have lost this.

Here's what I added to a project to bring this back, using the same SilverStripe_log facility that I believe was used previously.

In a new file, e.g. mysite/_config/logging.yml:

SilverStripe\Core\Injector\Injector:
  Psr\Log\LoggerInterface:
    calls:
      SyslogHandler: [ pushHandler, [ %$SyslogHandler ] ]
  SyslogHandler:
    class: Monolog\Handler\SyslogHandler
    constructor:
      - 'SilverStripe_log'

This is enough to push log messages through to CWP's Graylog system.

Migrated from GitLab

cc @chillu

See commit message and https://gitlab.cwp.govt.nz/cwp/cwp/merge_requests/65. It's technically an API change, I'm not sure how we handle semver with CWP. Until recently, defining a new index would work in parallel to the built-in CWP index, so its likely that even early adopters of custom indexes won't need to change anything in their custom code.

Comments

Usernames adjusted for GitHub

Damian Mooyman @tractorcow commented 2 years ago

Do you still need this? It'll need a rebase.

Robbie Averill @robbieaverill commented about a minute ago

I found this very confusing when getting into Solr. +1 for the change. I'll move this to GitHub as an issue and we can attack it for SS4 version.

Overview

CWP password hashing for versions 2.4 and above was updated in #51 to use SHA-512 rather than blowfish for password hashing.

However, the current iteration configuration is below recommended security guidelines. The security of PBKDF2 comes from the number of rounds performed. The current configuration, as shown in the source code below, sets a default cost of 10000 iterations. Security recommendations for PBKDF2 are to increase the cost to an acceptable delay and resource consumption level above 27,000. This will depend on server architecture and available resources.

Path: vendor\cwp\cwp-core\src\PasswordEncryptor\PBKDF2.php
protected $iterations = 10000;

Recommendation

• Configure the PBKDF2 default iterations to use any tolerable cost above 27,000.
• Confirm with TSP that server architecture can support this.

Note: User's passwords would need to be rehashed after this configuration change, so we should make this change before CWP 2.4 is released, so the old iteration doesn't make it to production.

The 2.8 build is broken because the composer root version for that branch is contradictory

Pull requests

• #97

Since these are being introduced into silverstripe/recipe-core (which cwp/cwp-recipe-core requires) we can remove some of the duplication.

We'd still need to keep the complexity rules in this module's _config.php file though.

See: silverstripe/recipe-core#17