Skip to content

Commit 47f8d50

Browse files
polarathenegeorglauterbach
polarathene
and
georglauterbach
authored
fix: Ensure configs are sanitized for parsing (docker-mailserver#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 <[email protected]> --------- Co-authored-by: Georg Lauterbach <[email protected]>
1 parent 22c6dae commit 47f8d50

7 files changed

Lines changed: 92 additions & 31 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,10 @@ The most noteworthy change of this release is the update of the container's base
3838
- **Tests:**
3939
- 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))
4040

41+
### Fixes
42+
43+
- 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))
44+
4145
## [v13.3.1](https://github.com/docker-mailserver/docker-mailserver/releases/tag/v13.3.1)
4246

4347
### Fixes

‎target/scripts/helpers/accounts.sh‎

Lines changed: 0 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -19,16 +19,9 @@ function _create_accounts() {
1919
_create_masters
2020

2121
if [[ -f ${DATABASE_ACCOUNTS} ]]; then
22-
_log 'trace' "Checking file line endings"
23-
sed -i 's|\r||g' "${DATABASE_ACCOUNTS}"
24-
2522
_log 'trace' "Regenerating postfix user list"
2623
echo "# WARNING: this file is auto-generated. Modify ${DATABASE_ACCOUNTS} to edit the user list." > /etc/postfix/vmailbox
2724

28-
# checking that ${DATABASE_ACCOUNTS} ends with a newline
29-
# shellcheck disable=SC1003
30-
sed -i -e '$a\' "${DATABASE_ACCOUNTS}"
31-
3225
chown dovecot:dovecot "${DOVECOT_USERDB_FILE}"
3326
chmod 640 "${DOVECOT_USERDB_FILE}"
3427

@@ -158,15 +151,8 @@ function _create_masters() {
158151

159152
local DATABASE_DOVECOT_MASTERS='/tmp/docker-mailserver/dovecot-masters.cf'
160153
if [[ -f ${DATABASE_DOVECOT_MASTERS} ]]; then
161-
_log 'trace' "Checking file line endings"
162-
sed -i 's|\r||g' "${DATABASE_DOVECOT_MASTERS}"
163-
164154
_log 'trace' "Regenerating dovecot masters list"
165155

166-
# checking that ${DATABASE_DOVECOT_MASTERS} ends with a newline
167-
# shellcheck disable=SC1003
168-
sed -i -e '$a\' "${DATABASE_DOVECOT_MASTERS}"
169-
170156
chown dovecot:dovecot "${DOVECOT_MASTERDB_FILE}"
171157
chmod 640 "${DOVECOT_MASTERDB_FILE}"
172158

‎target/scripts/helpers/aliases.sh‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -12,11 +12,6 @@ function _handle_postfix_virtual_config() {
1212
local DATABASE_VIRTUAL=/tmp/docker-mailserver/postfix-virtual.cf
1313

1414
if [[ -f ${DATABASE_VIRTUAL} ]]; then
15-
# fixing old virtual user file
16-
if grep -q ",$" "${DATABASE_VIRTUAL}"; then
17-
sed -i -e "s|, |,|g" -e "s|,$||g" "${DATABASE_VIRTUAL}"
18-
fi
19-
2015
cp -f "${DATABASE_VIRTUAL}" /etc/postfix/virtual
2116
else
2217
_log 'debug' "'${DATABASE_VIRTUAL}' not provided - no mail alias/forward created"

‎target/scripts/helpers/utils.sh‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,9 +17,44 @@ function _escape_for_sed() {
1717
# Returns input after filtering out lines that are:
1818
# empty, white-space, comments (`#` as the first non-whitespace character)
1919
function _get_valid_lines_from_file() {
20+
_convert_crlf_to_lf_if_necessary "${1}"
21+
_append_final_newline_if_missing "${1}"
22+
2023
grep --extended-regexp --invert-match "^\s*$|^\s*#" "${1}" || true
2124
}
2225

26+
# This is to sanitize configs from users that unknowingly introduced CRLF:
27+
function _convert_crlf_to_lf_if_necessary() {
28+
if [[ $(file "${1}") =~ 'CRLF' ]]; then
29+
_log 'warn' "File '${1}' contains CRLF line-endings"
30+
31+
if [[ -w ${1} ]]; then
32+
_log 'debug' 'Converting CRLF to LF'
33+
sed -i 's|\r||g' "${1}"
34+
else
35+
_log 'warn' "File '${1}' is not writable - cannot change CRLF to LF"
36+
fi
37+
fi
38+
}
39+
40+
# This is to sanitize configs from users that unknowingly removed the end-of-file LF:
41+
function _append_final_newline_if_missing() {
42+
# Correctly detect a missing final newline and fix it:
43+
# https://stackoverflow.com/questions/38746/how-to-detect-file-ends-in-newline#comment82380232_25749716
44+
# https://unix.stackexchange.com/questions/31947/how-to-add-a-newline-to-the-end-of-a-file/441200#441200
45+
# https://unix.stackexchange.com/questions/159557/how-to-non-invasively-test-for-write-access-to-a-file
46+
if [[ $(tail -c1 "${1}" | wc -l) -eq 0 ]]; then
47+
# Avoid fixing when the destination is read-only:
48+
if [[ -w ${1} ]]; then
49+
printf '\n' >> "${1}"
50+
51+
_log 'info' "File '${1}' was missing a final newline - this has been fixed"
52+
else
53+
_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!"
54+
fi
55+
fi
56+
}
57+
2358
# Provide the name of an environment variable to this function
2459
# and it will return its value stored in /etc/dms-settings
2560
function _get_dms_env_value() {

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

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -109,29 +109,29 @@ function _setup_postfix_late() {
109109
function __postfix__setup_override_configuration() {
110110
__postfix__log 'debug' 'Overriding / adjusting configuration with user-supplied values'
111111

112-
if [[ -f /tmp/docker-mailserver/postfix-main.cf ]]; then
113-
cat /tmp/docker-mailserver/postfix-main.cf >>/etc/postfix/main.cf
112+
local OVERRIDE_CONFIG_POSTFIX_MAIN='/tmp/docker-mailserver/postfix-main.cf'
113+
if [[ -f ${OVERRIDE_CONFIG_POSTFIX_MAIN} ]]; then
114+
cat "${OVERRIDE_CONFIG_POSTFIX_MAIN}" >>/etc/postfix/main.cf
114115
_adjust_mtime_for_postfix_maincf
115116

116117
# do not directly output to 'main.cf' as this causes a read-write-conflict
117118
postconf -n >/tmp/postfix-main-new.cf 2>/dev/null
118119

119120
mv /tmp/postfix-main-new.cf /etc/postfix/main.cf
120121
_adjust_mtime_for_postfix_maincf
121-
__postfix__log 'trace' "Adjusted '/etc/postfix/main.cf' according to '/tmp/docker-mailserver/postfix-main.cf'"
122+
__postfix__log 'trace' "Adjusted '/etc/postfix/main.cf' according to '${OVERRIDE_CONFIG_POSTFIX_MAIN}'"
122123
else
123-
__postfix__log 'trace' "No extra Postfix settings loaded because optional '/tmp/docker-mailserver/postfix-main.cf' was not provided"
124+
__postfix__log 'trace' "No extra Postfix settings loaded because optional '${OVERRIDE_CONFIG_POSTFIX_MAIN}' was not provided"
124125
fi
125126

126-
if [[ -f /tmp/docker-mailserver/postfix-master.cf ]]; then
127+
local OVERRIDE_CONFIG_POSTFIX_MASTER='/tmp/docker-mailserver/postfix-master.cf'
128+
if [[ -f ${OVERRIDE_CONFIG_POSTFIX_MASTER} ]]; then
127129
while read -r LINE; do
128-
if [[ ${LINE} =~ ^[0-9a-z] ]]; then
129-
postconf -P "${LINE}"
130-
fi
131-
done < /tmp/docker-mailserver/postfix-master.cf
132-
__postfix__log 'trace' "Adjusted '/etc/postfix/master.cf' according to '/tmp/docker-mailserver/postfix-master.cf'"
130+
[[ ${LINE} =~ ^[0-9a-z] ]] && postconf -P "${LINE}"
131+
done < <(_get_valid_lines_from_file "${OVERRIDE_CONFIG_POSTFIX_MASTER}")
132+
__postfix__log 'trace' "Adjusted '/etc/postfix/master.cf' according to '${OVERRIDE_CONFIG_POSTFIX_MASTER}'"
133133
else
134-
__postfix__log 'trace' "No extra Postfix settings loaded because optional '/tmp/docker-mailserver/postfix-master.cf' was not provided"
134+
__postfix__log 'trace' "No extra Postfix settings loaded because optional '${OVERRIDE_CONFIG_POSTFIX_MASTER}' was not provided"
135135
fi
136136
}
137137

‎test/helper/setup.bash‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -102,7 +102,6 @@ function _init_with_defaults() {
102102

103103
# The config volume cannot be read-only as some data needs to be written at container startup
104104
#
105-
# - two sed failures (unknown lines)
106105
# - dovecot-quotas.cf (setup-stack.sh:_setup_dovecot_quotas)
107106
# - postfix-aliases.cf (setup-stack.sh:_setup_postfix_aliases)
108107
# 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‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,3 +70,45 @@ SOURCE_BASE_PATH="${REPOSITORY_ROOT:?Expected REPOSITORY_ROOT to be set}/target/
7070
assert_failure
7171
assert_output --partial "ENV var name must be provided to _env_var_expect_integer"
7272
}
73+
74+
@test '(utils.sh) _convert_crlf_to_lf_if_necessary' {
75+
# shellcheck source=../../../../../target/scripts/helpers/log.sh
76+
source "${SOURCE_BASE_PATH}/log.sh"
77+
# shellcheck source=../../../../../target/scripts/helpers/utils.sh
78+
source "${SOURCE_BASE_PATH}/utils.sh"
79+
80+
# Create a temporary file in the BATS test-case folder:
81+
local TMP_DMS_CONFIG=$(mktemp -p "${BATS_TEST_TMPDIR}" -t 'dms_XXX.cf')
82+
# A file with mixed line-endings including CRLF:
83+
echo -en 'line one\nline two\r\n' > "${TMP_DMS_CONFIG}"
84+
85+
# Confirm CRLF detected:
86+
run file "${TMP_DMS_CONFIG}"
87+
assert_output --partial 'CRLF'
88+
89+
# Helper method detects and fixes:
90+
_convert_crlf_to_lf_if_necessary "${TMP_DMS_CONFIG}"
91+
run file "${TMP_DMS_CONFIG}"
92+
refute_output --partial 'CRLF'
93+
}
94+
95+
@test '(utils.sh) _append_final_newline_if_missing' {
96+
# shellcheck source=../../../../../target/scripts/helpers/log.sh
97+
source "${SOURCE_BASE_PATH}/log.sh"
98+
# shellcheck source=../../../../../target/scripts/helpers/utils.sh
99+
source "${SOURCE_BASE_PATH}/utils.sh"
100+
101+
# Create a temporary file in the BATS test-case folder:
102+
local TMP_DMS_CONFIG=$(mktemp -p "${BATS_TEST_TMPDIR}" -t 'dms_XXX.cf')
103+
# A file missing a final newline:
104+
echo -en 'line one\nline two' > "${TMP_DMS_CONFIG}"
105+
106+
# Confirm missing newline:
107+
run bash -c "tail -c 1 '${TMP_DMS_CONFIG}' | wc -l"
108+
assert_output '0'
109+
110+
# Helper method detects and fixes:
111+
_append_final_newline_if_missing "${TMP_DMS_CONFIG}"
112+
run bash -c "tail -c 1 '${TMP_DMS_CONFIG}' | wc -l"
113+
assert_output '1'
114+
}

0 commit comments

Comments
 (0)