Be careful with # comments in .env files. Do we need a .env syntax checker?

McFateM commented 5 years ago

Issue description

Based on personal experience as well as IslandoraCon workshop participant issues, we might want to build in an early check for # comments in our .env files. In my testing I think I have verified that a line like DRUPAL_DB_USER=public # Just confirming that comments after a space are allowed here, where there's a # comment on the same line as the variable definition, can cause odd eventual errors when a user runs the Drupal build script. Recovery from said errors, apparently due to incomplete database initialization, seems to require wiping out all Docker volumes and bind mounts, and starting over.

During the IslandoraCon workshop more than one user "inserted" their usernames or passwords to the local.env variable definitions rather than "replacing" the comments as instructed. It's an easy habit to adopt since many, like myself, wish to preserve comments from the original file just to remember and document where changes were made.

At IslandoraCon I also saw similar issues that may have been due to people putting special characters in their usernames and passwords, things like exclamation points, semi-colons, ampersands and carets.

Perhaps we can parse and check for these potential issues before they cause so much of a headache?

For an issue, describe steps to reproduce the issue

Begin with a working local instance of ISLE, and modify the .env file, presumably the local.env copy, to include an inline comment like shown above, bring the working stack down, and back up. You may see no change due to persistence of volumes. However, if you reset Docker or take steps to remove all volumes, and restart the stack, errors appear when in the docker exec... running of the Drupal build tools script.

System setup (OS information, software versions, etc): ISLE v.1.3.0 in both OSX and Windows, I believe.

What's the expected result?

Local ISLE startup without odd errors.

What's the actual result?

In my last test I got lots of this during the Drupal build tools run:

Command pm-enable needs a higher bootstrap level to run - you will need to invoke drush from a more functional Drupal environment to run this command.          [error]
The drush command 'en islandora_fits' could not be executed.                                                                                                    [error]
Drush was not able to start (bootstrap) the Drupal database.                                                                                                    [error]

bseeger commented 5 years ago

An alternative method is to provide some default passwords so that it just works out of the box. If they don't replace passwords, then they end up with a working system, just not a very secure one.

bseeger commented 5 years ago

I have the same thoughts concerning the default domain name in the local.sh script as well as default information (same as local.env) for the settings.php file. If the user runs the system then it will function, just not with a very secure setup. And I think this would be fine for a local system.

ysuarez commented 5 years ago

@McFateM I have reproduced my problem since last week with trying to install 1.20 and now 1.30. I had been using an "@" sign within my DRUPAL_ADMIN_USER value the whole time. (We use that format at my work.)

Should I put in an issue for this? or a PR to the comments for that value with a warning?

McFateM commented 5 years ago

@ysuarez Thank you so much for troubleshooting this and posting Yamil! I think your addition here is sufficient so no need to open another issue or PR.

I did a quick check on the web and there are numerous posts indicating that Linux variable names should contain only letters, numbers and underscores... NOTHING else. I'm not sure of the same for variable values, but probably a good convention to adhere too anyway.

ysuarez commented 4 years ago

@McFateM (and others like @dwk2 , @marksandford @g7morris ) Last week on the ISLE mailing list there was someone that had a similar issue to mine, in their case they were using a "$" in their mysql root password. This makes me wonder if we want to add a short warning in the Step 4/5: Create New Users and Passwords by Editing "local.env" File that quotes what you mentioned above.

For example, in this section...

Review carefully as some comments request that you replace with ...26 alpha-numeric characters while others request that you create an ...easy to read but short database name.

Change it to something like

Review carefully as some comments request that you replace with ...26 alpha-numeric characters while others request that you create an ...easy to read but short database name.
- NOTE: When the comments suggest alpha-numeric values remember that means you can only use the following characters "a-z A-Z 0-9" if not you may have issues when installing parts of ISLE.

Alternatively we could add an entry to the "ISLE Installation: Troubleshooting" page with an entry that cites some of the errors you can get with Drush or connecting to MySQL when you fail to use alpha-numeric characters in the relevant spots in the .ENV file.

I can create a pull request for either or both approaches (in all the corresponding demo/local/staging/prod files), and link it to this issue or another, if folks think either one of these suggestions is worth pursuing.

Finally, I was wondering if .ENV files allow the use of quotes in variable declarations, i.e. DRUPAL_ADMIN_USER='myp@ss'? If so I wonder if that would then allow the use of some additional non-alpha-numeric characters in the demo/local/staging/prod.env files. I can see some potential users of ISLE would want to have the flexibility to use some non-alpha-numeric in usernames or passwords to feel like they are creating more secure passwords. Though of course you can have secure passwords without using non-alphanumeric characters, for example if the passwords are long.

Islandora-Collaboration-Group / ISLE