Discourse-setup fails on SMTP username with a /

bug
1

Continuing the discussion from Move from standalone container to separate web and data containers:

Bug description

When running discourse-setup with an SMTP username that contains a /, the script fails:

sed: -e expression #1, char 74: unknown option to `s’
DISCOURSE_SMTP_USER_NAME change failed.

Expected behavior

SMTP usernames containing / should be accepted.

/ is a valid character for an SMTP username, according to RFC3696:

Without quotes, local-parts may consist of any combination of
alphabetic characters, digits, or any of the special characters

 ! # $ % & ' * + - / = ?  ^ _ ` . { | } ~

Corrective path

The script fails on a sed command that, by default, uses / as the separator.
Since the colon (:) is not an acceptable character for the local-part of an email address, it may be used instead of /, without any side-effect another character such as a comma or semi-colon could bring (except that colons in the sed patterns must be escaped — note that using ] instead would remove the need to escape colons):

  • line 589:
    smtp_domain=$(echo $notification_email | sed -e "s/.*@//") should be
    smtp_domain=$(echo $notification_email | sed -e "s:.*@::")
  • line 648: DISCOURSE_DEVELOPER_EMAILS uses , to separate email addresses, so it could safely use : as well
  • line 675 is the source of my bug:
    sed -i -e "s/^ #\?DISCOURSE_SMTP_USER_NAME:.*/ DISCOURSE_SMTP_USER_NAME: $smtp_user_name/w $changelog" $web_file may be
    sed -i -e "s:^ #\?DISCOURSE_SMTP_USER_NAME\:.*: DISCOURSE_SMTP_USER_NAME\: $smtp_user_name:w $changelog" $web_file or
    sed -i -e "s]^ #\?DISCOURSE_SMTP_USER_NAME:.*] DISCOURSE_SMTP_USER_NAME: $smtp_user_name]w $changelog" $web_file
  • ditto for line 684 (DISCOURSE_NOTIFICATION_EMAIL)
4 Likes
2

I’m not in control of that code, but I wrote it long ago. That’s a great solution. I should have thought of that. I’d guess that a PR would be welcome.

4 Likes
3

Unfortunately I lost access to my Github account. They ask me for some code and I don’t use the linked email anymore, so I simply stopped using that platform except to clone public code.

1 Like
4

Thanks for the report, putting pr-welcome in case someone is able to submit a patch.

You could always just post a .diff here and then have a community member take the credit :slight_smile:

4 Likes
5

Here is a patch:

Fix sed commands to allow for RFC3696 compliant email addresses

discourse-setup.fix-272514.patch.gz (601 Bytes)

diff --git a/discourse-setup b/discourse-setup
index 0daafcb..121982e 100755
--- a/discourse-setup
+++ b/discourse-setup
@@ -586,7 +586,7 @@ ask_user_for_config() {
     fi

     # set smtp_domain default value here rather than use Rails default of localhost
-    smtp_domain=$(echo $notification_email | sed -e "s/.*@//")
+    smtp_domain=$(echo $notification_email | sed -e "s].*@]]")

     if [ ! -z $letsencrypt_account_email ]
     then
@@ -645,7 +645,7 @@ ask_user_for_config() {
     update_ok="n"
   fi

-  sed -i -e "s/^  DISCOURSE_DEVELOPER_EMAILS:.*/  DISCOURSE_DEVELOPER_EMAILS: \'$developer_emails\'/w $changelog" $web_file
+  sed -i -e "s]^  DISCOURSE_DEVELOPER_EMAILS:.*]  DISCOURSE_DEVELOPER_EMAILS: \'$developer_emails\']w $changelog" $web_file
   if [ -s $changelog ]
   then
     rm $changelog
@@ -672,7 +672,7 @@ ask_user_for_config() {
     update_ok="n"
   fi

-  sed -i -e "s/^  #\?DISCOURSE_SMTP_USER_NAME:.*/  DISCOURSE_SMTP_USER_NAME: $smtp_user_name/w $changelog" $web_file
+  sed -i -e "s]^  #\?DISCOURSE_SMTP_USER_NAME:.*]  DISCOURSE_SMTP_USER_NAME: $smtp_user_name]w $changelog" $web_file
   if [ -s $changelog ]
   then
     rm $changelog
@@ -681,7 +681,7 @@ ask_user_for_config() {
     update_ok="n"
   fi

-  sed -i -e "s/^  #\?DISCOURSE_NOTIFICATION_EMAIL:.*/  DISCOURSE_NOTIFICATION_EMAIL: $notification_email/w $changelog" $web_file
+  sed -i -e "s]^  #\?DISCOURSE_NOTIFICATION_EMAIL:.*]  DISCOURSE_NOTIFICATION_EMAIL: $notification_email]w $changelog" $web_file
   if [ -s $changelog ]
   then
     rm $changelog
@@ -745,7 +745,7 @@ ask_user_for_config() {
   then
     echo "Enabling Let's Encrypt"
   fi
-  sed -i -e "s/^  #\?LETSENCRYPT_ACCOUNT_EMAIL:.*/  LETSENCRYPT_ACCOUNT_EMAIL: $letsencrypt_account_email/w $changelog" $web_file
+  sed -i -e "s]^  #\?LETSENCRYPT_ACCOUNT_EMAIL:.*]  LETSENCRYPT_ACCOUNT_EMAIL: $letsencrypt_account_email]w $changelog" $web_file
   if [ -s $changelog ]
   then
     rm $changelog
3 Likes
6

Did you mean to leave the slash there on that one?

2 Likes
7

Thank you for this nice catch! I updated the patch.

3 Likes