Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CRM_Utils_Check - Soften messages for read-only extensionsDir #11895

Merged
merged 3 commits into from Mar 29, 2018

Conversation

totten
Copy link
Member

@totten totten commented Mar 28, 2018

Overview

The extensionsDir is a configurable folder where extension source-code can be written.

The folder may be managed with a few policies:

  • Web-writable -- Allow the web-based admin to perform download/upgrade functions. (Ex: A small NGO where the web designer also installed Civi.)
  • Read-only -- Allow a sysadmin/developer to manage the folder (with CLI/API/cv/git/etc), but don't allow web-based admin to perform download/upgrade functions. (Ex: A public demo site where visitors can preview the admin functionality -- but shouldn't be able to install their favorite crypto miner.)
  • Disable -- Don't allow any extensions to be loaded through this folder.

The web-writable policy is generally the default (because it's easier to get extensions and apply security updates), but it's no panacea. This PR improves messaging when someone has a different policy.

Before

There are redundant messages ("Directory not writeable"), and they strongly push the admin toward web-writable policy.

screen shot 2018-03-28 at 3 16 38 pm

After

There is only one message ("Read-Only Extensions"). It still encourages web-writable policy, but it lowers the severity and presents it a choice ("if you want X, do Y").

screen shot 2018-03-28 at 3 35 49 pm

Comments

A few folks have raised related conversations before. Please feel free to comment or thumbs-up/down. CC @xurizaemon @bgm @agh1 @MegaphoneJon

When you have a non-writeable extensions directory, *two* status checks will
copmlain about it (`checkDirsWritable`, `checkExtensions`).  Between the two,
`checkExtensions` is smarter.
There are competing schools of thought on whether extension folders should be web-writable:

* Sometimes, the most active (or the only) admins are web-based. Making the folder writeable lets them keep extensions up-to-date through the web UI.
  This includes applying security-fixes for extensions. Thus, sites with writeable extdir are harder to attack (more secure).
* Sometimes, the most active (or the only) admins don't use the web-based admin UI, and they don't trust any web-based users to do administration.
  They don't want the folder to be writeable.
* If there's a flaw that allows writing to the filesystem, it could be escalated to writing+executing code. Thus, sites with read-only
  extdir are harder to attack (more secure).

This commit tries to accept each scenario as valid -- but communicate
better.  Instead of flatly describing the read-only dir as erroreous,
present a warning with some choice/trade-off.
@seamuslee001
Copy link
Contributor

👍

@agh1
Copy link
Contributor

agh1 commented Mar 28, 2018

I like this: having the folder read-only will stand in the way of a feature (in-app extension installs and upgrades), but that feature isn't necessary for day-to-day operations and it might be intentionally blocked. I'm actually on the fence about whether it really should be a notice: a warning implies something is wrong, while a notice says it's merely out of the ordinary.

@seamuslee001
Copy link
Contributor

Agree with Andrew i think it should be a notice not a warning

@eileenmcnaughton
Copy link
Contributor

This change is contained within the extensions check subsystem & I think the above endorsements are enough to merit merging

@eileenmcnaughton eileenmcnaughton merged commit db5c67e into civicrm:master Mar 29, 2018
@totten totten deleted the master-ext-check branch March 29, 2018 02:35
@herbdool
Copy link
Contributor

herbdool commented Jul 4, 2018

Looks like it got merged as a warning, not a notice. I would have preferred it be a notice too.

Another reason to keep it unwritable is because we need to patch extensions and we can't fork and use our own private source if it was web-writable.

@eileenmcnaughton
Copy link
Contributor

@herbdool the fix softened it to a warning - I'm happy to accept a patch that softens further if people think that makes sense - I think I do agree with that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
6 participants