fix: Ensure configs are sanitized for parsing (#3819)

* chore: Detect missing final newline in configs read

These lines will be not be processed by `read`, emit a warning to raise awareness.

* fix: Ensure parsed config has final newline appended (when possible)

This functionality was handled in `accounts.sh` via a similar sed command (that the linked references also offer).

`printf` is better for this, no shellcheck comment required either.

We additionally don't attempt to modify files that are read-only.

* fix: Ensure parsed configs have CRLF to LF corrected (where possible)

Likewise, this runtime fix was only covering two config files. It now applies to all callers of this method.

* fix: Sanitize `postfix-master.cf` via helper

This feature should have been using the helper to avoid user error from their config updates accidentally introducing subtle breakage implicitly (due to CRLF or missing final newline).

* tests: Add test cases for new helpers

* tests:  `rm` is redundant when using `BATS_TEST_TMPDIR`

This temporary directory is created and removed implicitly. Even after a test failure.

* chore: Remove old `postfix-virtual.cf` migration logic

This was introduced in 2018, there should be no one needing to rely on this anymore?

* tests: Remove comment on sed failure concern

* chore: Add entry to `CHANGELOG.md`

* Apply suggestions from code review

Co-authored-by: Georg Lauterbach <44545919+georglauterbach@users.noreply.github.com>

---------

Co-authored-by: Georg Lauterbach <44545919+georglauterbach@users.noreply.github.com>

CHANGELOG.md

@@ -38,6 +38,10 @@ The most noteworthy change of this release is the update of the container's base
 - **Tests:**
   - Refactored helper methods for sending e-mails with specific `Message-ID` headers and the helpers for retrieving + filtering logs, which together help isolate logs relevant to specific mail when multiple mails have been processed within a single test. ([#3786](https://github.com/docker-mailserver/docker-mailserver/pull/3786))
 
+### Fixes
+
+- DMS config files that are parsed line by line are now more robust to parse by detecting and fixing line-endings ([#3819](https://github.com/docker-mailserver/docker-mailserver/pull/3819))
+
 ## [v13.3.1](https://github.com/docker-mailserver/docker-mailserver/releases/tag/v13.3.1)
 
 ### Fixes

target/scripts/helpers/accounts.sh

@@ -19,16 +19,9 @@ function _create_accounts() {
   _create_masters
 
   if [[ -f ${DATABASE_ACCOUNTS} ]]; then
-    _log 'trace' "Checking file line endings"
-    sed -i 's|\r||g' "${DATABASE_ACCOUNTS}"
-
     _log 'trace' "Regenerating postfix user list"
     echo "# WARNING: this file is auto-generated. Modify ${DATABASE_ACCOUNTS} to edit the user list." > /etc/postfix/vmailbox
 
-    # checking that ${DATABASE_ACCOUNTS} ends with a newline
-    # shellcheck disable=SC1003
-    sed -i -e '$a\' "${DATABASE_ACCOUNTS}"
-
     chown dovecot:dovecot "${DOVECOT_USERDB_FILE}"
     chmod 640 "${DOVECOT_USERDB_FILE}"
 
@@ -158,15 +151,8 @@ function _create_masters() {
 
   local DATABASE_DOVECOT_MASTERS='/tmp/docker-mailserver/dovecot-masters.cf'
   if [[ -f ${DATABASE_DOVECOT_MASTERS} ]]; then
-    _log 'trace' "Checking file line endings"
-    sed -i 's|\r||g' "${DATABASE_DOVECOT_MASTERS}"
-
     _log 'trace' "Regenerating dovecot masters list"
 
-    # checking that ${DATABASE_DOVECOT_MASTERS} ends with a newline
-    # shellcheck disable=SC1003
-    sed -i -e '$a\' "${DATABASE_DOVECOT_MASTERS}"
-
     chown dovecot:dovecot "${DOVECOT_MASTERDB_FILE}"
     chmod 640 "${DOVECOT_MASTERDB_FILE}"
 

target/scripts/helpers/aliases.sh

@@ -12,11 +12,6 @@ function _handle_postfix_virtual_config() {
   local DATABASE_VIRTUAL=/tmp/docker-mailserver/postfix-virtual.cf
 
   if [[ -f ${DATABASE_VIRTUAL} ]]; then
-    # fixing old virtual user file
-    if grep -q ",$" "${DATABASE_VIRTUAL}"; then
-      sed -i -e "s|, |,|g" -e "s|,$||g" "${DATABASE_VIRTUAL}"
-    fi
-
     cp -f "${DATABASE_VIRTUAL}" /etc/postfix/virtual
   else
     _log 'debug' "'${DATABASE_VIRTUAL}' not provided - no mail alias/forward created"

target/scripts/helpers/utils.sh

@@ -17,9 +17,44 @@ function _escape_for_sed() {
 # Returns input after filtering out lines that are:
 # empty, white-space, comments (`#` as the first non-whitespace character)
 function _get_valid_lines_from_file() {
+  _convert_crlf_to_lf_if_necessary "${1}"
+  _append_final_newline_if_missing "${1}"
+
   grep --extended-regexp --invert-match "^\s*$|^\s*#" "${1}" || true
 }
 
+# This is to sanitize configs from users that unknowingly introduced CRLF:
+function _convert_crlf_to_lf_if_necessary() {
+  if [[ $(file "${1}") =~ 'CRLF' ]]; then
+    _log 'warn' "File '${1}' contains CRLF line-endings"
+
+    if [[ -w ${1} ]]; then
+      _log 'debug' 'Converting CRLF to LF'
+      sed -i 's|\r||g' "${1}"
+    else
+      _log 'warn' "File '${1}' is not writable - cannot change CRLF to LF"
+    fi
+  fi
+}
+
+# This is to sanitize configs from users that unknowingly removed the end-of-file LF:
+function _append_final_newline_if_missing() {
+  # Correctly detect a missing final newline and fix it:
+  # https://stackoverflow.com/questions/38746/how-to-detect-file-ends-in-newline#comment82380232_25749716
+  # https://unix.stackexchange.com/questions/31947/how-to-add-a-newline-to-the-end-of-a-file/441200#441200
+  # https://unix.stackexchange.com/questions/159557/how-to-non-invasively-test-for-write-access-to-a-file
+  if [[ $(tail -c1 "${1}" | wc -l) -eq 0 ]]; then
+    # Avoid fixing when the destination is read-only:
+    if [[ -w ${1} ]]; then
+      printf '\n' >> "${1}"
+
+      _log 'info' "File '${1}' was missing a final newline - this has been fixed"
+    else
+      _log 'warn' "File '${1}' is missing a final newline - it is not writable, hence it was not fixed - the last line will not be processed!"
+    fi
+  fi
+}
+
 # Provide the name of an environment variable to this function
 # and it will return its value stored in /etc/dms-settings
 function _get_dms_env_value() {

target/scripts/startup/setup.d/postfix.sh

@@ -109,8 +109,9 @@ function _setup_postfix_late() {
 function __postfix__setup_override_configuration() {
   __postfix__log 'debug' 'Overriding / adjusting configuration with user-supplied values'
 
-  if [[ -f /tmp/docker-mailserver/postfix-main.cf ]]; then
-    cat /tmp/docker-mailserver/postfix-main.cf >>/etc/postfix/main.cf
+  local OVERRIDE_CONFIG_POSTFIX_MAIN='/tmp/docker-mailserver/postfix-main.cf'
+  if [[ -f ${OVERRIDE_CONFIG_POSTFIX_MAIN} ]]; then
+    cat "${OVERRIDE_CONFIG_POSTFIX_MAIN}" >>/etc/postfix/main.cf
     _adjust_mtime_for_postfix_maincf
 
     # do not directly output to 'main.cf' as this causes a read-write-conflict
@@ -118,20 +119,19 @@ function __postfix__setup_override_configuration() {
 
     mv /tmp/postfix-main-new.cf /etc/postfix/main.cf
     _adjust_mtime_for_postfix_maincf
-    __postfix__log 'trace' "Adjusted '/etc/postfix/main.cf' according to '/tmp/docker-mailserver/postfix-main.cf'"
+    __postfix__log 'trace' "Adjusted '/etc/postfix/main.cf' according to '${OVERRIDE_CONFIG_POSTFIX_MAIN}'"
   else
-    __postfix__log 'trace' "No extra Postfix settings loaded because optional '/tmp/docker-mailserver/postfix-main.cf' was not provided"
+    __postfix__log 'trace' "No extra Postfix settings loaded because optional '${OVERRIDE_CONFIG_POSTFIX_MAIN}' was not provided"
   fi
 
-  if [[ -f /tmp/docker-mailserver/postfix-master.cf ]]; then
+  local OVERRIDE_CONFIG_POSTFIX_MASTER='/tmp/docker-mailserver/postfix-master.cf'
+  if [[ -f ${OVERRIDE_CONFIG_POSTFIX_MASTER} ]]; then
     while read -r LINE; do
-      if [[ ${LINE} =~ ^[0-9a-z] ]]; then
-        postconf -P "${LINE}"
-      fi
-    done < /tmp/docker-mailserver/postfix-master.cf
-    __postfix__log 'trace' "Adjusted '/etc/postfix/master.cf' according to '/tmp/docker-mailserver/postfix-master.cf'"
+      [[ ${LINE} =~ ^[0-9a-z] ]] && postconf -P "${LINE}"
+    done < <(_get_valid_lines_from_file "${OVERRIDE_CONFIG_POSTFIX_MASTER}")
+    __postfix__log 'trace' "Adjusted '/etc/postfix/master.cf' according to '${OVERRIDE_CONFIG_POSTFIX_MASTER}'"
   else
-    __postfix__log 'trace' "No extra Postfix settings loaded because optional '/tmp/docker-mailserver/postfix-master.cf' was not provided"
+    __postfix__log 'trace' "No extra Postfix settings loaded because optional '${OVERRIDE_CONFIG_POSTFIX_MASTER}' was not provided"
   fi
 }
 

test/helper/setup.bash

@@ -102,7 +102,6 @@ function _init_with_defaults() {
 
   # The config volume cannot be read-only as some data needs to be written at container startup
   #
-  # - two sed failures (unknown lines)
   # - dovecot-quotas.cf (setup-stack.sh:_setup_dovecot_quotas)
   # - postfix-aliases.cf (setup-stack.sh:_setup_postfix_aliases)
   # TODO: Check how many tests need write access. Consider using `docker create` + `docker cp` for easier cleanup.

test/tests/parallel/set3/scripts/helper_functions.bats

@@ -70,3 +70,45 @@ SOURCE_BASE_PATH="${REPOSITORY_ROOT:?Expected REPOSITORY_ROOT to be set}/target/
   assert_failure
   assert_output --partial "ENV var name must be provided to _env_var_expect_integer"
 }
+
+@test '(utils.sh) _convert_crlf_to_lf_if_necessary' {
+  # shellcheck source=../../../../../target/scripts/helpers/log.sh
+  source "${SOURCE_BASE_PATH}/log.sh"
+  # shellcheck source=../../../../../target/scripts/helpers/utils.sh
+  source "${SOURCE_BASE_PATH}/utils.sh"
+
+  # Create a temporary file in the BATS test-case folder:
+  local TMP_DMS_CONFIG=$(mktemp -p "${BATS_TEST_TMPDIR}" -t 'dms_XXX.cf')
+  # A file with mixed line-endings including CRLF:
+  echo -en 'line one\nline two\r\n' > "${TMP_DMS_CONFIG}"
+
+  # Confirm CRLF detected:
+  run file "${TMP_DMS_CONFIG}"
+  assert_output --partial 'CRLF'
+
+  # Helper method detects and fixes:
+  _convert_crlf_to_lf_if_necessary "${TMP_DMS_CONFIG}"
+  run file "${TMP_DMS_CONFIG}"
+  refute_output --partial 'CRLF'
+}
+
+@test '(utils.sh) _append_final_newline_if_missing' {
+  # shellcheck source=../../../../../target/scripts/helpers/log.sh
+  source "${SOURCE_BASE_PATH}/log.sh"
+  # shellcheck source=../../../../../target/scripts/helpers/utils.sh
+  source "${SOURCE_BASE_PATH}/utils.sh"
+
+  # Create a temporary file in the BATS test-case folder:
+  local TMP_DMS_CONFIG=$(mktemp -p "${BATS_TEST_TMPDIR}" -t 'dms_XXX.cf')
+  # A file missing a final newline:
+  echo -en 'line one\nline two' > "${TMP_DMS_CONFIG}"
+
+  # Confirm missing newline:
+  run bash -c "tail -c 1 '${TMP_DMS_CONFIG}' | wc -l"
+  assert_output '0'
+
+  # Helper method detects and fixes:
+  _append_final_newline_if_missing "${TMP_DMS_CONFIG}"
+  run bash -c "tail -c 1 '${TMP_DMS_CONFIG}' | wc -l"
+  assert_output '1'
+}